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..77836f6784c --- /dev/null +++ b/changelog/v1.17.0-beta27/refactor-transformation-validation.yaml @@ -0,0 +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. + Introduce a new Validator that validates an envoy config by running it by envoy in validate mode. diff --git a/projects/gloo/pkg/bootstrap/bootstrap_validation.go b/projects/gloo/pkg/bootstrap/bootstrap_validation.go index c703b2b9752..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,15 +32,9 @@ func getEnvoyPath() string { func ValidateBootstrap( ctx context.Context, - settings *v1.Settings, 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..1a94af426f1 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,15 +62,17 @@ 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 { + 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{ - validationLruCache: lru.New(1024), + validator: validator.New(ExtensionName, FilterName, + validator.WithCounters(mCacheHits, mCacheMisses)), } } @@ -536,36 +534,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, p.settings, FilterName, transformations) - p.validationLruCache.Add(transformHash, err) - if err != nil { - return err - } - return nil + return p.validator.ValidateConfig(ctx, transformations) } func (p *Plugin) getTransformations( 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 new file mode 100644 index 00000000000..a92d930c2da --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/options.go @@ -0,0 +1,41 @@ +package validator + +import ( + "fmt" + + "github.com/solo-io/gloo/pkg/utils" + "go.opencensus.io/stats" +) + +type config struct { + cacheHits *stats.Int64Measure + cacheMisses *stats.Int64Measure + cacheSize int +} + +type Option func(*config) + +func WithCounters(cacheHits, cacheMisses *stats.Int64Measure) Option { + return func(s *config) { + s.cacheHits = cacheHits + s.cacheMisses = cacheMisses + } +} + +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) + } + 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..e6feff983a9 --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/validator.go @@ -0,0 +1,86 @@ +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" +) + +// 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). +// 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 + + // CacheLength returns the returns the number of items in the cache + CacheLength() int +} + +type validator struct { + filterName string + // lruCache is a map of: (config hash) -> error state + // this is usually a typed error but may be an untyped nil interface + lruCache *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, + lruCache: lru.New(cfg.cacheSize), + 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 proto has already been validated, return the result + if err, ok := v.lruCache.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 + } + utils.MeasureOne( + ctx, + v.cacheMisses, + ) + + err = bootstrap.ValidateBootstrap(ctx, v.filterName, config) + 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..48bdf0919ed --- /dev/null +++ b/projects/gloo/pkg/plugins/utils/validator/validator_test.go @@ -0,0 +1,98 @@ +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() { + + 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") + + validator := New("test", "test", + WithCounters(mCacheHits, mCacheMisses)) + Expect(validator.CacheLength()).To(Equal(0)) + + 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)) + }) + + 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))) + }) + +})