From 4a09b629f6d89b586b5820b4b599b57071997a13 Mon Sep 17 00:00:00 2001 From: David Jumani Date: Wed, 8 May 2024 22:49:06 -0400 Subject: [PATCH 1/9] refactor: Make bootstrap.ValidateBootstrap independant of the DisableTransformationValidation setting --- .../refactor-transformation-validation.yaml | 3 +++ projects/gloo/pkg/bootstrap/bootstrap_validation.go | 5 ----- projects/gloo/pkg/plugins/transformation/plugin.go | 10 ++++++---- 3 files changed, 9 insertions(+), 9 deletions(-) create mode 100644 changelog/v1.17.0-beta26/refactor-transformation-validation.yaml diff --git a/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml b/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml new file mode 100644 index 00000000000..e8a3cd50694 --- /dev/null +++ b/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml @@ -0,0 +1,3 @@ +changelog: +- type: NON_USER_FACING + description: Refactor `bootstrap.ValidateBootstrap()` by moving the `DisableTransformationValidation` check into the transformation plugin. This way the `bootstrap.ValidateBootstrap()` can be used in other areas independent of this setting. diff --git a/projects/gloo/pkg/bootstrap/bootstrap_validation.go b/projects/gloo/pkg/bootstrap/bootstrap_validation.go index c703b2b9752..e4654e146af 100644 --- a/projects/gloo/pkg/bootstrap/bootstrap_validation.go +++ b/projects/gloo/pkg/bootstrap/bootstrap_validation.go @@ -37,11 +37,6 @@ func ValidateBootstrap( filterName string, msg proto.Message, ) error { - // If the user has disabled transformation validation, then always return nil - if settings.GetGateway().GetValidation().GetDisableTransformationValidation().GetValue() { - return nil - } - bootstrapYaml, err := buildPerFilterBootstrapYaml(filterName, msg) if err != nil { return err diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index 805a91aead7..23a22ad2c8b 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -560,12 +560,14 @@ func (p *Plugin) validateTransformation( ) } + // If the user has disabled transformation validation, then always return nil + if p.settings.GetGateway().GetValidation().GetDisableTransformationValidation().GetValue() { + return nil + } + err = bootstrap.ValidateBootstrap(ctx, p.settings, FilterName, transformations) p.validationLruCache.Add(transformHash, err) - if err != nil { - return err - } - return nil + return err } func (p *Plugin) getTransformations( From f7f8a1724b5f57b3e9dca1bee5da5427279ddb20 Mon Sep 17 00:00:00 2001 From: David Jumani Date: Wed, 8 May 2024 22:55:09 -0400 Subject: [PATCH 2/9] update function signature --- projects/gloo/pkg/bootstrap/bootstrap_validation.go | 2 -- projects/gloo/pkg/plugins/transformation/plugin.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/projects/gloo/pkg/bootstrap/bootstrap_validation.go b/projects/gloo/pkg/bootstrap/bootstrap_validation.go index e4654e146af..71fa04ec7e7 100644 --- a/projects/gloo/pkg/bootstrap/bootstrap_validation.go +++ b/projects/gloo/pkg/bootstrap/bootstrap_validation.go @@ -16,7 +16,6 @@ import ( "github.com/golang/protobuf/proto" "github.com/golang/protobuf/ptypes/any" "github.com/rotisserie/eris" - v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" "github.com/solo-io/gloo/projects/gloo/pkg/utils" "github.com/solo-io/go-utils/contextutils" ) @@ -33,7 +32,6 @@ func getEnvoyPath() string { func ValidateBootstrap( ctx context.Context, - settings *v1.Settings, filterName string, msg proto.Message, ) error { diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index 23a22ad2c8b..1c7d542d830 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -565,7 +565,7 @@ func (p *Plugin) validateTransformation( return nil } - err = bootstrap.ValidateBootstrap(ctx, p.settings, FilterName, transformations) + err = bootstrap.ValidateBootstrap(ctx, FilterName, transformations) p.validationLruCache.Add(transformHash, err) return err } From d3b7851be042f6eaac112ee5fcaf350e7185059c Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Thu, 9 May 2024 21:05:53 +0000 Subject: [PATCH 3/9] Adding changelog file to new location --- .../v1.17.0-beta27/refactor-transformation-validation.yaml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/v1.17.0-beta27/refactor-transformation-validation.yaml diff --git a/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml b/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml new file mode 100644 index 00000000000..e8a3cd50694 --- /dev/null +++ b/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml @@ -0,0 +1,3 @@ +changelog: +- type: NON_USER_FACING + description: Refactor `bootstrap.ValidateBootstrap()` by moving the `DisableTransformationValidation` check into the transformation plugin. This way the `bootstrap.ValidateBootstrap()` can be used in other areas independent of this setting. From 3bfa0838b7d8214fbe2ff99ad378dcd6fbf95fc0 Mon Sep 17 00:00:00 2001 From: changelog-bot Date: Thu, 9 May 2024 21:05:54 +0000 Subject: [PATCH 4/9] Deleting changelog file from old location --- .../v1.17.0-beta26/refactor-transformation-validation.yaml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 changelog/v1.17.0-beta26/refactor-transformation-validation.yaml diff --git a/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml b/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml deleted file mode 100644 index e8a3cd50694..00000000000 --- a/changelog/v1.17.0-beta26/refactor-transformation-validation.yaml +++ /dev/null @@ -1,3 +0,0 @@ -changelog: -- type: NON_USER_FACING - description: Refactor `bootstrap.ValidateBootstrap()` by moving the `DisableTransformationValidation` check into the transformation plugin. This way the `bootstrap.ValidateBootstrap()` can be used in other areas independent of this setting. From a7c2f97741ff26f8d95f27900b5f2d2a48bb74df Mon Sep 17 00:00:00 2001 From: David Jumani Date: Thu, 9 May 2024 23:26:27 -0400 Subject: [PATCH 5/9] introduce a validator --- .../refactor-transformation-validation.yaml | 4 +- .../gloo/pkg/plugins/transformation/plugin.go | 54 ++++--------- .../pkg/plugins/utils/validator/options.go | 38 ++++++++++ .../pkg/plugins/utils/validator/validator.go | 76 +++++++++++++++++++ 4 files changed, 132 insertions(+), 40 deletions(-) create mode 100644 projects/gloo/pkg/plugins/utils/validator/options.go create mode 100644 projects/gloo/pkg/plugins/utils/validator/validator.go diff --git a/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml b/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml index e8a3cd50694..77836f6784c 100644 --- a/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml +++ b/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml @@ -1,3 +1,5 @@ changelog: - type: NON_USER_FACING - description: Refactor `bootstrap.ValidateBootstrap()` by moving the `DisableTransformationValidation` check into the transformation plugin. This way the `bootstrap.ValidateBootstrap()` can be used in other areas independent of this setting. + description: > + Refactor `bootstrap.ValidateBootstrap()` by moving the `DisableTransformationValidation` check into the transformation plugin. This way the `bootstrap.ValidateBootstrap()` can be used in other areas independent of this setting. + Introduce a new Validator that validates an envoy config by running it by envoy in validate mode. diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index 1c7d542d830..082aaddd7b1 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -7,9 +7,8 @@ import ( "github.com/golang/protobuf/proto" "github.com/rotisserie/eris" - "github.com/solo-io/go-utils/contextutils" + "github.com/solo-io/gloo/projects/gloo/pkg/plugins/utils/validator" "google.golang.org/protobuf/types/known/wrapperspb" - "k8s.io/utils/lru" envoy_config_route_v3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" envoy_type_matcher_v3 "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3" @@ -22,7 +21,6 @@ import ( v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/core/matchers" "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/transformation" - "github.com/solo-io/gloo/projects/gloo/pkg/bootstrap" "github.com/solo-io/gloo/projects/gloo/pkg/plugins" "github.com/solo-io/gloo/projects/gloo/pkg/plugins/pluginutils" ) @@ -49,8 +47,6 @@ var ( UnknownTransformationType = func(transformation interface{}) error { return fmt.Errorf("unknown transformation type %T", transformation) } - mCacheHits = utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_hits", "The number of cache hits while validating transformation config") - mCacheMisses = utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_misses", "The number of cache misses while validating transformation config") ) type TranslateTransformationFn func(*transformation.Transformation, *wrapperspb.BoolValue, *wrapperspb.BoolValue) (*envoytransformation.Transformation, error) @@ -66,16 +62,12 @@ type Plugin struct { TranslateTransformation TranslateTransformationFn settings *v1.Settings logRequestResponseInfo bool - // validationLruCache is a map of: (transformation hash) -> error state - // this is usually a typed error but may be an untyped nil interface - validationLruCache *lru.Cache - escapeCharacters *wrapperspb.BoolValue + escapeCharacters *wrapperspb.BoolValue + validator validator.Validator } func NewPlugin() *Plugin { - return &Plugin{ - validationLruCache: lru.New(1024), - } + return &Plugin{} } func (p *Plugin) Name() string { @@ -91,6 +83,16 @@ func (p *Plugin) Init(params plugins.InitParams) { p.TranslateTransformation = TranslateTransformation p.escapeCharacters = params.Settings.GetGloo().GetTransformationEscapeCharacters() p.logRequestResponseInfo = params.Settings.GetGloo().GetLogTransformationRequestResponseInfo().GetValue() + // Skip validation if disabled + if !p.settings.GetGateway().GetValidation().GetDisableTransformationValidation().GetValue() { + mCacheHits := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_hits", "The number of cache hits while validating transformation config") + mCacheMisses := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_misses", "The number of cache misses while validating transformation config") + + p.validator = validator.New(ExtensionName, FilterName, + validator.WithCacheHitCounter(mCacheHits), + validator.WithCacheMissCounter(mCacheMisses)) + } + } func mergeFunc(tx *envoytransformation.RouteTransformations) pluginutils.ModifyFunc { @@ -536,38 +538,12 @@ func (p *Plugin) validateTransformation( ctx context.Context, transformations *envoytransformation.RouteTransformations, ) error { - - transformHash, err := transformations.Hash(nil) - if err != nil { - contextutils.LoggerFrom(ctx).DPanicf("error hashing transformation, should never happen: %v", err) - return err - } - - // This transformation has already been validated, return the result - if err, ok := p.validationLruCache.Get(transformHash); ok { - utils.MeasureOne( - ctx, - mCacheHits, - ) - // Error may be nil here since it's just the cached result - // so return it as a nil err after cast worst case. - errCasted, _ := err.(error) - return errCasted - } else { - utils.MeasureOne( - ctx, - mCacheMisses, - ) - } - // If the user has disabled transformation validation, then always return nil if p.settings.GetGateway().GetValidation().GetDisableTransformationValidation().GetValue() { return nil } - err = bootstrap.ValidateBootstrap(ctx, FilterName, transformations) - p.validationLruCache.Add(transformHash, err) - return err + return p.validator.ValidateConfig(ctx, transformations) } func (p *Plugin) getTransformations( diff --git a/projects/gloo/pkg/plugins/utils/validator/options.go b/projects/gloo/pkg/plugins/utils/validator/options.go new file mode 100644 index 00000000000..1d2b61be1d6 --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/options.go @@ -0,0 +1,38 @@ +package validator + +import ( + "fmt" + + "github.com/solo-io/gloo/pkg/utils" + "go.opencensus.io/stats" +) + +type config struct { + cacheHits *stats.Int64Measure + cacheMisses *stats.Int64Measure +} + +type Option func(*config) + +func WithCacheHitCounter(counter *stats.Int64Measure) Option { + return func(s *config) { + s.cacheHits = counter + } +} + +func WithCacheMissCounter(counter *stats.Int64Measure) Option { + return func(s *config) { + s.cacheMisses = counter + } +} + +func processOptions(name string, options ...Option) *config { + cfg := &config{ + cacheHits: utils.MakeSumCounter(fmt.Sprintf("gloo.solo.io/%s_validation_cache_hits", name), fmt.Sprintf("The number of cache hits while validating %s config", name)), + cacheMisses: utils.MakeSumCounter(fmt.Sprintf("gloo.solo.io/%s_validation_cache_misses", name), fmt.Sprintf("The number of cache misses while validating %s config", name)), + } + for _, option := range options { + option(cfg) + } + return cfg +} diff --git a/projects/gloo/pkg/plugins/utils/validator/validator.go b/projects/gloo/pkg/plugins/utils/validator/validator.go new file mode 100644 index 00000000000..8427431b355 --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -0,0 +1,76 @@ +package validator + +import ( + "context" + "hash" + + "github.com/solo-io/gloo/pkg/utils" + "github.com/solo-io/gloo/projects/gloo/pkg/bootstrap" + "github.com/solo-io/go-utils/contextutils" + "go.opencensus.io/stats" + "google.golang.org/protobuf/runtime/protoiface" + "k8s.io/utils/lru" +) + +// Validator validates an envoy config by running it by envoy in validate mode. This requires the envoy binary to be present at $ENVOY_BINARY_PATH (defaults to /usr/local/bin/envoy) +type Validator interface { + // ValidateConfig validates the given envoy config and returns any out and error from envoy. Returns nil if the envoy binary is not found. + ValidateConfig(ctx context.Context, config HashableProtoMessage) error +} + +type validator struct { + filterName string + // validationLruCache is a map of: (config hash) -> error state + // this is usually a typed error but may be an untyped nil interface + validationLruCache *lru.Cache + // Counter to increment on cache hits + cacheHits *stats.Int64Measure + // Counter to increment on cache misses + cacheMisses *stats.Int64Measure +} + +// New returns a new Validator +func New(name string, filterName string, opts ...Option) validator { + cfg := processOptions(name, opts...) + return validator{ + filterName: filterName, + validationLruCache: lru.New(1024), + cacheHits: cfg.cacheHits, + cacheMisses: cfg.cacheMisses, + } +} + +// HashableProtoMessage defines a ProtoMessage that can be hashed. Useful when passing different ProtoMessages objects that need to be hashed. +type HashableProtoMessage interface { + protoiface.MessageV1 + Hash(hasher hash.Hash64) (uint64, error) +} + +func (v validator) ValidateConfig(ctx context.Context, config HashableProtoMessage) error { + hash, err := config.Hash(nil) + if err != nil { + contextutils.LoggerFrom(ctx).DPanicf("error hashing the config, should never happen: %v", err) + return err + } + + // This transformation has already been validated, return the result + if err, ok := v.validationLruCache.Get(hash); ok { + utils.MeasureOne( + ctx, + v.cacheHits, + ) + // Error may be nil here since it's just the cached result + // so return it as a nil err after cast worst case. + errCasted, _ := err.(error) + return errCasted + } else { + utils.MeasureOne( + ctx, + v.cacheMisses, + ) + } + + err = bootstrap.ValidateBootstrap(ctx, v.filterName, config) + v.validationLruCache.Add(hash, err) + return err +} From d4eb00fd285055b2b4fe340b7a04c226685fd6dc Mon Sep 17 00:00:00 2001 From: David Jumani Date: Thu, 9 May 2024 23:30:11 -0400 Subject: [PATCH 6/9] Add cache sizing option --- projects/gloo/pkg/plugins/utils/validator/options.go | 8 ++++++++ projects/gloo/pkg/plugins/utils/validator/validator.go | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/projects/gloo/pkg/plugins/utils/validator/options.go b/projects/gloo/pkg/plugins/utils/validator/options.go index 1d2b61be1d6..5a9cff634ce 100644 --- a/projects/gloo/pkg/plugins/utils/validator/options.go +++ b/projects/gloo/pkg/plugins/utils/validator/options.go @@ -10,6 +10,7 @@ import ( type config struct { cacheHits *stats.Int64Measure cacheMisses *stats.Int64Measure + cacheSize int } type Option func(*config) @@ -26,10 +27,17 @@ func WithCacheMissCounter(counter *stats.Int64Measure) Option { } } +func WithCacheSize(size int) Option { + return func(s *config) { + s.cacheSize = size + } +} + func processOptions(name string, options ...Option) *config { cfg := &config{ cacheHits: utils.MakeSumCounter(fmt.Sprintf("gloo.solo.io/%s_validation_cache_hits", name), fmt.Sprintf("The number of cache hits while validating %s config", name)), cacheMisses: utils.MakeSumCounter(fmt.Sprintf("gloo.solo.io/%s_validation_cache_misses", name), fmt.Sprintf("The number of cache misses while validating %s config", name)), + cacheSize: DefaultCacheSize, } for _, option := range options { option(cfg) diff --git a/projects/gloo/pkg/plugins/utils/validator/validator.go b/projects/gloo/pkg/plugins/utils/validator/validator.go index 8427431b355..f1e3ad6b197 100644 --- a/projects/gloo/pkg/plugins/utils/validator/validator.go +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -12,7 +12,10 @@ import ( "k8s.io/utils/lru" ) -// Validator validates an envoy config by running it by envoy in validate mode. This requires the envoy binary to be present at $ENVOY_BINARY_PATH (defaults to /usr/local/bin/envoy) +const DefaultCacheSize int = 1024 + +// Validator validates an envoy config by running it by envoy in validate mode. This requires the envoy binary to be present at $ENVOY_BINARY_PATH (defaults to /usr/local/bin/envoy). +// Results are cached via an LRU cache for performance type Validator interface { // ValidateConfig validates the given envoy config and returns any out and error from envoy. Returns nil if the envoy binary is not found. ValidateConfig(ctx context.Context, config HashableProtoMessage) error @@ -34,7 +37,7 @@ func New(name string, filterName string, opts ...Option) validator { cfg := processOptions(name, opts...) return validator{ filterName: filterName, - validationLruCache: lru.New(1024), + validationLruCache: lru.New(cfg.cacheSize), cacheHits: cfg.cacheHits, cacheMisses: cfg.cacheMisses, } From 21e1acfb30b4270d15c9f0d815d8350a36e4005c Mon Sep 17 00:00:00 2001 From: David Jumani Date: Fri, 10 May 2024 13:09:12 -0400 Subject: [PATCH 7/9] address comments --- .../gloo/pkg/plugins/transformation/plugin.go | 19 ++++++++----------- .../pkg/plugins/utils/validator/validator.go | 11 +++++------ 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index 082aaddd7b1..cf2fa06f23e 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -67,7 +67,14 @@ type Plugin struct { } func NewPlugin() *Plugin { - return &Plugin{} + mCacheHits := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_hits", "The number of cache hits while validating transformation config") + mCacheMisses := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_misses", "The number of cache misses while validating transformation config") + + return &Plugin{ + validator: validator.New(ExtensionName, FilterName, + validator.WithCacheHitCounter(mCacheHits), + validator.WithCacheMissCounter(mCacheMisses)), + } } func (p *Plugin) Name() string { @@ -83,16 +90,6 @@ func (p *Plugin) Init(params plugins.InitParams) { p.TranslateTransformation = TranslateTransformation p.escapeCharacters = params.Settings.GetGloo().GetTransformationEscapeCharacters() p.logRequestResponseInfo = params.Settings.GetGloo().GetLogTransformationRequestResponseInfo().GetValue() - // Skip validation if disabled - if !p.settings.GetGateway().GetValidation().GetDisableTransformationValidation().GetValue() { - mCacheHits := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_hits", "The number of cache hits while validating transformation config") - mCacheMisses := utils.MakeSumCounter("gloo.solo.io/transformation_validation_cache_misses", "The number of cache misses while validating transformation config") - - p.validator = validator.New(ExtensionName, FilterName, - validator.WithCacheHitCounter(mCacheHits), - validator.WithCacheMissCounter(mCacheMisses)) - } - } func mergeFunc(tx *envoytransformation.RouteTransformations) pluginutils.ModifyFunc { diff --git a/projects/gloo/pkg/plugins/utils/validator/validator.go b/projects/gloo/pkg/plugins/utils/validator/validator.go index f1e3ad6b197..92617156e11 100644 --- a/projects/gloo/pkg/plugins/utils/validator/validator.go +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -56,7 +56,7 @@ func (v validator) ValidateConfig(ctx context.Context, config HashableProtoMessa return err } - // This transformation has already been validated, return the result + // This proto has already been validated, return the result if err, ok := v.validationLruCache.Get(hash); ok { utils.MeasureOne( ctx, @@ -66,12 +66,11 @@ func (v validator) ValidateConfig(ctx context.Context, config HashableProtoMessa // so return it as a nil err after cast worst case. errCasted, _ := err.(error) return errCasted - } else { - utils.MeasureOne( - ctx, - v.cacheMisses, - ) } + utils.MeasureOne( + ctx, + v.cacheMisses, + ) err = bootstrap.ValidateBootstrap(ctx, v.filterName, config) v.validationLruCache.Add(hash, err) From c539aab51c311600f0bf6d99a5a6b8d734f2782c Mon Sep 17 00:00:00 2001 From: David Jumani Date: Mon, 13 May 2024 09:18:50 -0400 Subject: [PATCH 8/9] add tests --- .../gloo/pkg/plugins/transformation/plugin.go | 3 +- .../pkg/plugins/transformation/plugin_test.go | 52 +++++++++++++- .../transformation_suite_test.go | 2 +- .../pkg/plugins/utils/validator/options.go | 11 +-- .../pkg/plugins/utils/validator/validator.go | 26 ++++--- .../utils/validator/validator_suite_test.go | 13 ++++ .../plugins/utils/validator/validator_test.go | 71 +++++++++++++++++++ 7 files changed, 156 insertions(+), 22 deletions(-) create mode 100644 projects/gloo/pkg/plugins/utils/validator/validator_suite_test.go create mode 100644 projects/gloo/pkg/plugins/utils/validator/validator_test.go diff --git a/projects/gloo/pkg/plugins/transformation/plugin.go b/projects/gloo/pkg/plugins/transformation/plugin.go index cf2fa06f23e..1a94af426f1 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin.go +++ b/projects/gloo/pkg/plugins/transformation/plugin.go @@ -72,8 +72,7 @@ func NewPlugin() *Plugin { return &Plugin{ validator: validator.New(ExtensionName, FilterName, - validator.WithCacheHitCounter(mCacheHits), - validator.WithCacheMissCounter(mCacheMisses)), + validator.WithCounters(mCacheHits, mCacheMisses)), } } diff --git a/projects/gloo/pkg/plugins/transformation/plugin_test.go b/projects/gloo/pkg/plugins/transformation/plugin_test.go index 4320a190658..ae763e60983 100644 --- a/projects/gloo/pkg/plugins/transformation/plugin_test.go +++ b/projects/gloo/pkg/plugins/transformation/plugin_test.go @@ -1,4 +1,4 @@ -package transformation_test +package transformation import ( "context" @@ -17,7 +17,6 @@ import ( "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/core/matchers" "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/transformation" "github.com/solo-io/gloo/projects/gloo/pkg/plugins" - . "github.com/solo-io/gloo/projects/gloo/pkg/plugins/transformation" "github.com/solo-io/gloo/projects/gloo/pkg/utils" skMatchers "github.com/solo-io/solo-kit/test/matchers" ) @@ -1082,4 +1081,53 @@ var _ = Describe("Plugin", func() { }) }) + Context("cache validation", func() { + var p *Plugin + var processRoute func() + var processAnotherRoute func() + + BeforeEach(func() { + p = NewPlugin() + p.Init(plugins.InitParams{Ctx: ctx, Settings: &v1.Settings{Gloo: &v1.GlooOptions{RemoveUnusedFilters: &wrapperspb.BoolValue{Value: false}}}}) + + processRouteWithValue := func(value bool) { + err := p.ProcessRoute(plugins.RouteParams{ + VirtualHostParams: plugins.VirtualHostParams{ + Params: plugins.Params{ + Ctx: ctx, + }, + }, + }, &v1.Route{ + Options: &v1.RouteOptions{ + Transformations: &transformation.Transformations{ + ClearRouteCache: value, + }, + }, + }, &envoy_config_route_v3.Route{}) + Expect(err).ToNot(HaveOccurred()) + } + + processRoute = func() { + processRouteWithValue(true) + } + processAnotherRoute = func() { + processRouteWithValue(false) + } + }) + + It("reuses the cache", func() { + processRoute() + Expect(p.validator.CacheLength()).To(Equal(1)) + + // When re-initializing the plugin, the cache is not cleared + p.Init(plugins.InitParams{Ctx: ctx, Settings: &v1.Settings{Gloo: &v1.GlooOptions{RemoveUnusedFilters: &wrapperspb.BoolValue{Value: false}}}}) + Expect(p.validator.CacheLength()).To(Equal(1)) + + // The cache is still not cleared + p.Init(plugins.InitParams{Ctx: ctx, Settings: &v1.Settings{Gloo: &v1.GlooOptions{RemoveUnusedFilters: &wrapperspb.BoolValue{Value: false}}}}) + processAnotherRoute() + Expect(p.validator.CacheLength()).To(Equal(2)) + }) + + }) }) diff --git a/projects/gloo/pkg/plugins/transformation/transformation_suite_test.go b/projects/gloo/pkg/plugins/transformation/transformation_suite_test.go index bdbec7f3fd9..da1f544e947 100644 --- a/projects/gloo/pkg/plugins/transformation/transformation_suite_test.go +++ b/projects/gloo/pkg/plugins/transformation/transformation_suite_test.go @@ -1,4 +1,4 @@ -package transformation_test +package transformation import ( "testing" diff --git a/projects/gloo/pkg/plugins/utils/validator/options.go b/projects/gloo/pkg/plugins/utils/validator/options.go index 5a9cff634ce..a92d930c2da 100644 --- a/projects/gloo/pkg/plugins/utils/validator/options.go +++ b/projects/gloo/pkg/plugins/utils/validator/options.go @@ -15,15 +15,10 @@ type config struct { type Option func(*config) -func WithCacheHitCounter(counter *stats.Int64Measure) Option { +func WithCounters(cacheHits, cacheMisses *stats.Int64Measure) Option { return func(s *config) { - s.cacheHits = counter - } -} - -func WithCacheMissCounter(counter *stats.Int64Measure) Option { - return func(s *config) { - s.cacheMisses = counter + s.cacheHits = cacheHits + s.cacheMisses = cacheMisses } } diff --git a/projects/gloo/pkg/plugins/utils/validator/validator.go b/projects/gloo/pkg/plugins/utils/validator/validator.go index 92617156e11..569806a5430 100644 --- a/projects/gloo/pkg/plugins/utils/validator/validator.go +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -12,6 +12,7 @@ import ( "k8s.io/utils/lru" ) +// DefaultCacheSize defines the default size of the LRU cache used by the validator const DefaultCacheSize int = 1024 // Validator validates an envoy config by running it by envoy in validate mode. This requires the envoy binary to be present at $ENVOY_BINARY_PATH (defaults to /usr/local/bin/envoy). @@ -19,13 +20,16 @@ const DefaultCacheSize int = 1024 type Validator interface { // ValidateConfig validates the given envoy config and returns any out and error from envoy. Returns nil if the envoy binary is not found. ValidateConfig(ctx context.Context, config HashableProtoMessage) error + + // CacheLength returns the returns the number of items in the cache + CacheLength() int } type validator struct { filterName string - // validationLruCache is a map of: (config hash) -> error state + // lruCache is a map of: (config hash) -> error state // this is usually a typed error but may be an untyped nil interface - validationLruCache *lru.Cache + lruCache *lru.Cache // Counter to increment on cache hits cacheHits *stats.Int64Measure // Counter to increment on cache misses @@ -33,13 +37,13 @@ type validator struct { } // New returns a new Validator -func New(name string, filterName string, opts ...Option) validator { +func New(name string, filterName string, opts ...Option) Validator { cfg := processOptions(name, opts...) return validator{ - filterName: filterName, - validationLruCache: lru.New(cfg.cacheSize), - cacheHits: cfg.cacheHits, - cacheMisses: cfg.cacheMisses, + filterName: filterName, + lruCache: lru.New(cfg.cacheSize), + cacheHits: cfg.cacheHits, + cacheMisses: cfg.cacheMisses, } } @@ -57,7 +61,7 @@ func (v validator) ValidateConfig(ctx context.Context, config HashableProtoMessa } // This proto has already been validated, return the result - if err, ok := v.validationLruCache.Get(hash); ok { + if err, ok := v.lruCache.Get(hash); ok { utils.MeasureOne( ctx, v.cacheHits, @@ -73,6 +77,10 @@ func (v validator) ValidateConfig(ctx context.Context, config HashableProtoMessa ) err = bootstrap.ValidateBootstrap(ctx, v.filterName, config) - v.validationLruCache.Add(hash, err) + v.lruCache.Add(hash, err) return err } + +func (v validator) CacheLength() int { + return v.lruCache.Len() +} diff --git a/projects/gloo/pkg/plugins/utils/validator/validator_suite_test.go b/projects/gloo/pkg/plugins/utils/validator/validator_suite_test.go new file mode 100644 index 00000000000..d2622c2fa0c --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/validator_suite_test.go @@ -0,0 +1,13 @@ +package validator + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestValidator(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Validation Suite") +} diff --git a/projects/gloo/pkg/plugins/utils/validator/validator_test.go b/projects/gloo/pkg/plugins/utils/validator/validator_test.go new file mode 100644 index 00000000000..2f9abd1d098 --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/validator_test.go @@ -0,0 +1,71 @@ +package validator + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/solo-io/gloo/pkg/utils" + "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/transformation" + "go.opencensus.io/stats/view" +) + +// Validation against envoy can not be tested here as it is designed to pass if the envoy binary is not found. So we test the cache and metrics instead. +// Ref: https://github.com/solo-io/gloo/blob/7e503dea039fa69211232c83bd07f8c169df0d45/projects/gloo/pkg/bootstrap/bootstrap_validation.go#L53 +// Validation is tested in the enterprise gateway kube2e tests +var _ = Describe("Validator", func() { + It("Updates the stats", func() { + cacheHitsName := "gloo.solo.io/test_validation_cache_hits" + cacheMissesName := "gloo.solo.io/test_validation_cache_misses" + mCacheHits := utils.MakeSumCounter(cacheHitsName, "The number of cache hits while validating test config") + mCacheMisses := utils.MakeSumCounter(cacheMissesName, "The number of cache misses while validating test config") + ctx := context.Background() + + validator := New("test", "test", + WithCounters(mCacheHits, mCacheMisses)) + Expect(validator.CacheLength()).To(Equal(0)) + + testProto := &transformation.TransformationStages{ + InheritTransformation: false, + } + + validator.ValidateConfig(ctx, testProto) + + // On the first validation, the cacheMiss = 1, cacheHits = empty, cache length = 1 + rows, err := view.RetrieveData(cacheMissesName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + rows, err = view.RetrieveData(cacheHitsName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).To(BeEmpty()) + Expect(validator.CacheLength()).To(Equal(1)) + + validator.ValidateConfig(ctx, testProto) + + // On the next validation of the same proto, the cacheMiss = 1, cacheHits = 1, cache length = 1 + rows, err = view.RetrieveData(cacheMissesName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + rows, err = view.RetrieveData(cacheHitsName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + Expect(validator.CacheLength()).To(Equal(1)) + + testProto.InheritTransformation = true + validator.ValidateConfig(ctx, testProto) + + // On the validation of another proto, the cacheMiss = 2, cacheHits = 1, cache length = 2 + rows, err = view.RetrieveData(cacheMissesName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(2))) + rows, err = view.RetrieveData(cacheHitsName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + Expect(validator.CacheLength()).To(Equal(2)) + }) +}) From bbec022f801e7f650b52e50ca085036cea0ceb0a Mon Sep 17 00:00:00 2001 From: David Jumani Date: Mon, 13 May 2024 09:29:06 -0400 Subject: [PATCH 9/9] more tests --- .../pkg/plugins/utils/validator/validator.go | 2 +- .../plugins/utils/validator/validator_test.go | 37 ++++++++++++++++--- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/projects/gloo/pkg/plugins/utils/validator/validator.go b/projects/gloo/pkg/plugins/utils/validator/validator.go index 569806a5430..e6feff983a9 100644 --- a/projects/gloo/pkg/plugins/utils/validator/validator.go +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -37,7 +37,7 @@ type validator struct { } // New returns a new Validator -func New(name string, filterName string, opts ...Option) Validator { +func New(name string, filterName string, opts ...Option) validator { cfg := processOptions(name, opts...) return validator{ filterName: filterName, diff --git a/projects/gloo/pkg/plugins/utils/validator/validator_test.go b/projects/gloo/pkg/plugins/utils/validator/validator_test.go index 2f9abd1d098..48bdf0919ed 100644 --- a/projects/gloo/pkg/plugins/utils/validator/validator_test.go +++ b/projects/gloo/pkg/plugins/utils/validator/validator_test.go @@ -14,21 +14,26 @@ import ( // Ref: https://github.com/solo-io/gloo/blob/7e503dea039fa69211232c83bd07f8c169df0d45/projects/gloo/pkg/bootstrap/bootstrap_validation.go#L53 // Validation is tested in the enterprise gateway kube2e tests var _ = Describe("Validator", func() { + + var ctx context.Context + var testProto = &transformation.TransformationStages{ + InheritTransformation: false, + } + + BeforeEach(func() { + ctx = context.Background() + }) + It("Updates the stats", func() { cacheHitsName := "gloo.solo.io/test_validation_cache_hits" cacheMissesName := "gloo.solo.io/test_validation_cache_misses" mCacheHits := utils.MakeSumCounter(cacheHitsName, "The number of cache hits while validating test config") mCacheMisses := utils.MakeSumCounter(cacheMissesName, "The number of cache misses while validating test config") - ctx := context.Background() validator := New("test", "test", WithCounters(mCacheHits, mCacheMisses)) Expect(validator.CacheLength()).To(Equal(0)) - testProto := &transformation.TransformationStages{ - InheritTransformation: false, - } - validator.ValidateConfig(ctx, testProto) // On the first validation, the cacheMiss = 1, cacheHits = empty, cache length = 1 @@ -68,4 +73,26 @@ var _ = Describe("Validator", func() { Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) Expect(validator.CacheLength()).To(Equal(2)) }) + + It("creates its own counters", func() { + validator := New("custom", "custom") + + cacheHitsName := "gloo.solo.io/custom_validation_cache_hits" + cacheMissesName := "gloo.solo.io/custom_validation_cache_misses" + + // validate twice to populate both caches + validator.ValidateConfig(ctx, testProto) + validator.ValidateConfig(ctx, testProto) + + // On validation of the same proto twice, the cacheMiss = 1, cacheHits = 1 + rows, err := view.RetrieveData(cacheMissesName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + rows, err = view.RetrieveData(cacheHitsName) + Expect(err).NotTo(HaveOccurred()) + Expect(rows).NotTo(BeEmpty()) + Expect(rows[0].Data.(*view.SumData).Value).To(Equal(float64(1))) + }) + })