From 2d72bd94cd75c825ea195b89b2f4473a0e75d94b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 1 Jun 2021 16:26:29 +0200 Subject: [PATCH 01/55] wip --- _meta/beat.yml | 5 ++++- apm-server.docker.yml | 5 ++++- apm-server.yml | 5 ++++- beater/api/mux.go | 1 + beater/beater.go | 13 +---------- beater/config/agentconfig.go | 15 +++++-------- beater/config/rum.go | 20 +++++++++-------- beater/config/sourcemapping.go | 34 +++++++++++++++++++++++++++++ sourcemap/fleet_store.go | 40 ++++++++++++++++++++++++++++++++++ sourcemap/store.go | 40 +++++++++++++++++++++++++++++----- 10 files changed, 139 insertions(+), 39 deletions(-) create mode 100644 beater/config/sourcemapping.go create mode 100644 sourcemap/fleet_store.go diff --git a/_meta/beat.yml b/_meta/beat.yml index 7363d568b8b..82ad0cc8a4d 100644 --- a/_meta/beat.yml +++ b/_meta/beat.yml @@ -244,9 +244,12 @@ apm-server: # Sourcemapping is enabled by default. #enabled: true - # Source maps are always fetched from Elasticsearch, by default using the output.elasticsearch configuration. + # Source maps may be fetched from Elasticsearch by using the + # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. + # Note: Configuring elasticsearch is not allowed if apm-server is being + # managed by Fleet. #elasticsearch: # Array of hosts to connect to. # Scheme and port can be left out and will be set to the default (`http` and `9200`). diff --git a/apm-server.docker.yml b/apm-server.docker.yml index d131f087a8b..af8bdde46d7 100644 --- a/apm-server.docker.yml +++ b/apm-server.docker.yml @@ -244,9 +244,12 @@ apm-server: # Sourcemapping is enabled by default. #enabled: true - # Source maps are always fetched from Elasticsearch, by default using the output.elasticsearch configuration. + # Source maps may be fetched from Elasticsearch by using the + # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. + # Note: Configuring elasticsearch is not allowed if apm-server is being + # managed by Fleet. #elasticsearch: # Array of hosts to connect to. # Scheme and port can be left out and will be set to the default (`http` and `9200`). diff --git a/apm-server.yml b/apm-server.yml index e7bb6c3a6ff..e20126132bd 100644 --- a/apm-server.yml +++ b/apm-server.yml @@ -244,9 +244,12 @@ apm-server: # Sourcemapping is enabled by default. #enabled: true - # Source maps are always fetched from Elasticsearch, by default using the output.elasticsearch configuration. + # Source maps may be fetched from Elasticsearch by using the + # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. + # Note: Configuring elasticsearch is not allowed if apm-server is being + # managed by Fleet. #elasticsearch: # Array of hosts to connect to. # Scheme and port can be left out and will be set to the default (`http` and `9200`). diff --git a/beater/api/mux.go b/beater/api/mux.go index 2ee9ba6ae47..97dd5c8ca0a 100644 --- a/beater/api/mux.go +++ b/beater/api/mux.go @@ -163,6 +163,7 @@ func (r *routeBuilder) rumV3IntakeHandler() (request.Handler, error) { } func (r *routeBuilder) sourcemapHandler() (request.Handler, error) { + // TODO: Turn this off, don't allow sourcemap upload in managed mode h := sourcemap.Handler(r.reporter) authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeSourcemapWrite.Action) return middleware.Wrap(h, sourcemapMiddleware(r.cfg, authHandler)...) diff --git a/beater/beater.go b/beater/beater.go index 719bd80f8db..917daa93b5f 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -22,7 +22,6 @@ import ( "net" "regexp" "runtime" - "strings" "sync" "time" @@ -44,7 +43,6 @@ import ( "github.com/elastic/beats/v7/libbeat/publisher/pipetool" "github.com/elastic/apm-server/beater/config" - "github.com/elastic/apm-server/elasticsearch" "github.com/elastic/apm-server/ingest/pipeline" logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/model" @@ -607,7 +605,7 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } if cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled && cfg.RumConfig.SourceMapping.ESConfig != nil { - store, err := newSourcemapStore(beatInfo, cfg.RumConfig.SourceMapping) + store, err := sourcemap.NewStore(beatInfo, cfg.RumConfig.SourceMapping) if err != nil { return nil, err } @@ -617,15 +615,6 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf return transformConfig, nil } -func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { - esClient, err := elasticsearch.NewClient(cfg.ESConfig) - if err != nil { - return nil, err - } - index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) - return sourcemap.NewStore(esClient, index, cfg.Cache.Expiration) -} - // WrapRunServerWithProcessors wraps runServer such that it wraps args.Reporter // with a function that event batches are first passed through the given processors // in order. diff --git a/beater/config/agentconfig.go b/beater/config/agentconfig.go index 63901fa4527..fb7e9751773 100644 --- a/beater/config/agentconfig.go +++ b/beater/config/agentconfig.go @@ -53,9 +53,6 @@ type AgentConfig struct { } func (s *AgentConfig) setup() error { - if !s.Service.isValid() { - return errInvalidAgentConfigServiceName - } if s.Config == nil { return errInvalidAgentConfigMissingConfig } @@ -74,20 +71,20 @@ func (s *AgentConfig) setup() error { type Service struct { Name string `config:"name"` Environment string `config:"environment"` + Version string `config:"version"` } // String implements the Stringer interface. func (s *Service) String() string { - var name, env string + var name, env, version string if s.Name != "" { name = "service.name=" + s.Name } if s.Environment != "" { env = "service.environment=" + s.Environment } - return strings.Join([]string{name, env}, " ") -} - -func (s *Service) isValid() bool { - return s.Name != "" || s.Environment != "" + if s.Version != "" { + version = "service.version=" + s.Version + } + return strings.Join([]string{name, env, version}, " ") } diff --git a/beater/config/rum.go b/beater/config/rum.go index 000280d9c3a..2671373b0c1 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -60,11 +60,12 @@ type EventRate struct { // SourceMapping holds sourecemap config information type SourceMapping struct { - Cache Cache `config:"cache"` - Enabled bool `config:"enabled"` - IndexPattern string `config:"index_pattern"` - ESConfig *elasticsearch.Config `config:"elasticsearch"` - esConfigured bool + Cache Cache `config:"cache"` + Enabled bool `config:"enabled"` + IndexPattern string `config:"index_pattern"` + ESConfig *elasticsearch.Config `config:"elasticsearch"` + SourceMapConfigs []SourceMapConfig `config:"source_maps"` + esConfigured bool } func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg *common.Config) error { @@ -115,10 +116,11 @@ func (s *SourceMapping) Unpack(inp *common.Config) error { func defaultSourcemapping() SourceMapping { return SourceMapping{ - Enabled: true, - Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, - IndexPattern: defaultSourcemapIndexPattern, - ESConfig: elasticsearch.DefaultConfig(), + Enabled: true, + Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, + IndexPattern: defaultSourcemapIndexPattern, + ESConfig: elasticsearch.DefaultConfig(), + SourceMapConfigs: []SourceMapConfig{defaultSourceMapConfig}, } } diff --git a/beater/config/sourcemapping.go b/beater/config/sourcemapping.go new file mode 100644 index 00000000000..611da69d363 --- /dev/null +++ b/beater/config/sourcemapping.go @@ -0,0 +1,34 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package config + +var defaultSourceMapConfig = SourceMapConfig{} + +type SourceMapConfig struct { + Service Service `config:"service"` + Bundle Bundle `config:"bundle"` + SourceMap SourceMap `config:"sourcemap"` +} + +type Bundle struct { + Filepath string `config:"filepath"` +} + +type SourceMap struct { + URL string `config:"url"` +} diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go new file mode 100644 index 00000000000..7953cb983a1 --- /dev/null +++ b/sourcemap/fleet_store.go @@ -0,0 +1,40 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package sourcemap + +import ( + "context" + "net/http" + + "github.com/elastic/apm-server/beater/config" +) + +type fleetStore struct { + // TODO: configurable client + c *http.Client + cfgs []config.SourceMapConfig +} + +func newFleetBackend(cfgs []config.SourceMapConfig) fleetStore { + return fleetStore{cfgs: cfgs, c: http.DefaultClient} +} + +func (f fleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { + // find the right sourcemap config, make request with its sourcemap.url + return "", nil +} diff --git a/sourcemap/store.go b/sourcemap/store.go index 462ffedd3d4..b923871d8f5 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -23,12 +23,14 @@ import ( "strings" "time" + "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/elasticsearch" "github.com/go-sourcemap/sourcemap" gocache "github.com/patrickmn/go-cache" "github.com/pkg/errors" + "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/libbeat/logp" logs "github.com/elastic/apm-server/log" @@ -45,20 +47,45 @@ var ( // Store holds information necessary to fetch a sourcemap, either from an Elasticsearch instance or an internal cache. type Store struct { cache *gocache.Cache - esStore *esStore + backend backend logger *logp.Logger } -// NewStore creates a new instance for fetching sourcemaps. The client and index parameters are needed to be able to -// fetch sourcemaps from Elasticsearch. The expiration time is used for the internal cache. -func NewStore(client elasticsearch.Client, index string, expiration time.Duration) (*Store, error) { +type backend interface { + fetch(ctx context.Context, name, version, path string) (string, error) +} + +// NewStore creates a new instance for fetching sourcemaps based on the +// provided config.SourceMapping. Only one of source_mapping.elasticsearch or +// sourcemapping.source_maps can be configured. +func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { + if cfg.ESConfig != nil && len(cfg.SourceMapConfigs) > 0 { + return nil, errors.New("configuring both source_mapping.elasticsearch and sourcemapping.source_maps not allowed") + } + expiration := cfg.Cache.Expiration if expiration < 0 { return nil, errInit } + + var b backend logger := logp.NewLogger(logs.Sourcemap) + + if len(cfg.SourceMapConfigs) == 0 { + logger.Info("no source maps configured, using esClient") + esClient, err := elasticsearch.NewClient(cfg.ESConfig) + if err != nil { + return nil, err + } + index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) + b = &esStore{client: esClient, index: index, logger: logger} + } else { + logger.Info("source maps configured, using fleet backend") + b = newFleetBackend(cfg.SourceMapConfigs) + } + return &Store{ cache: gocache.New(expiration, cleanupInterval(expiration)), - esStore: &esStore{client: client, index: index, logger: logger}, + backend: b, logger: logger, }, nil } @@ -74,8 +101,9 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str } // fetch from Elasticsearch and ensure caching for all non-temporary results - sourcemapStr, err := s.esStore.fetch(ctx, name, version, path) + sourcemapStr, err := s.backend.fetch(ctx, name, version, path) if err != nil { + // TODO: Check for errors from both stores if !strings.Contains(err.Error(), errMsgESFailure) { s.add(key, nil) } From 39344ed22350d3faaa8150e65d7f5a236b1b75f8 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 1 Jun 2021 17:23:32 +0200 Subject: [PATCH 02/55] fleet/es branching working --- beater/beater.go | 2 +- beater/config/rum.go | 14 ++++++++++++-- beater/config/sourcemapping.go | 2 -- sourcemap/store.go | 2 -- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index 917daa93b5f..f8ef1002013 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -604,7 +604,7 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf }, } - if cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled && cfg.RumConfig.SourceMapping.ESConfig != nil { + if cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled { store, err := sourcemap.NewStore(beatInfo, cfg.RumConfig.SourceMapping) if err != nil { return nil, err diff --git a/beater/config/rum.go b/beater/config/rum.go index 2671373b0c1..124b1a4796b 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -80,6 +80,16 @@ func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg return errors.Wrapf(err, "Invalid regex for `exclude_from_grouping`: ") } + if c.SourceMapping.esConfigured && len(c.SourceMapping.SourceMapConfigs) > 0 { + return errors.New("configuring both source_mapping.elasticsearch and sourcemapping.source_maps not allowed") + } + + // No need to unpack the ESConfig if we've set SourceMapConfigs + if len(c.SourceMapping.SourceMapConfigs) > 0 { + return nil + } + c.SourceMapping.ESConfig = elasticsearch.DefaultConfig() + var apiKey string if c.SourceMapping.esConfigured { if dataStreamsEnabled { @@ -119,8 +129,8 @@ func defaultSourcemapping() SourceMapping { Enabled: true, Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, IndexPattern: defaultSourcemapIndexPattern, - ESConfig: elasticsearch.DefaultConfig(), - SourceMapConfigs: []SourceMapConfig{defaultSourceMapConfig}, + ESConfig: nil, + SourceMapConfigs: []SourceMapConfig{}, } } diff --git a/beater/config/sourcemapping.go b/beater/config/sourcemapping.go index 611da69d363..f87b4b07d26 100644 --- a/beater/config/sourcemapping.go +++ b/beater/config/sourcemapping.go @@ -17,8 +17,6 @@ package config -var defaultSourceMapConfig = SourceMapConfig{} - type SourceMapConfig struct { Service Service `config:"service"` Bundle Bundle `config:"bundle"` diff --git a/sourcemap/store.go b/sourcemap/store.go index b923871d8f5..7c9ad164017 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -71,7 +71,6 @@ func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) if len(cfg.SourceMapConfigs) == 0 { - logger.Info("no source maps configured, using esClient") esClient, err := elasticsearch.NewClient(cfg.ESConfig) if err != nil { return nil, err @@ -79,7 +78,6 @@ func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) b = &esStore{client: esClient, index: index, logger: logger} } else { - logger.Info("source maps configured, using fleet backend") b = newFleetBackend(cfg.SourceMapConfigs) } From d2e4248604b4f3a88676d21543f1f35b46a411f1 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 3 Jun 2021 09:21:55 +0200 Subject: [PATCH 03/55] add apikey to fleetStore --- sourcemap/fleet_store.go | 10 +++++----- sourcemap/store.go | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 7953cb983a1..4a819474581 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -25,13 +25,13 @@ import ( ) type fleetStore struct { - // TODO: configurable client - c *http.Client - cfgs []config.SourceMapConfig + apiKey string + c *http.Client + cfgs []config.SourceMapConfig } -func newFleetBackend(cfgs []config.SourceMapConfig) fleetStore { - return fleetStore{cfgs: cfgs, c: http.DefaultClient} +func newFleetBackend(apiKey string, cfgs []config.SourceMapConfig, c *http.Client) fleetStore { + return fleetStore{apiKey: apiKey, cfgs: cfgs, c: c} } func (f fleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { diff --git a/sourcemap/store.go b/sourcemap/store.go index 7c9ad164017..25aa73ae0a4 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -20,6 +20,7 @@ package sourcemap import ( "context" "math" + "net/http" "strings" "time" @@ -78,7 +79,7 @@ func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) b = &esStore{client: esClient, index: index, logger: logger} } else { - b = newFleetBackend(cfg.SourceMapConfigs) + b = newFleetBackend(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, http.DefaultClient) } return &Store{ From ae01674dbb527491ccd3055e831f1fbb8362640d Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 3 Jun 2021 09:22:07 +0200 Subject: [PATCH 04/55] [sourcemap/store] comment about duplicated config --- sourcemap/store.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sourcemap/store.go b/sourcemap/store.go index 25aa73ae0a4..6cf72e3d578 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -60,6 +60,8 @@ type backend interface { // provided config.SourceMapping. Only one of source_mapping.elasticsearch or // sourcemapping.source_maps can be configured. func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { + // We're already checking for this in config parsing, so do we need it + // here a second time? if cfg.ESConfig != nil && len(cfg.SourceMapConfigs) > 0 { return nil, errors.New("configuring both source_mapping.elasticsearch and sourcemapping.source_maps not allowed") } From 5b8d875bc6a47c30edc20fd9ef8bcacd70ff271f Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 3 Jun 2021 12:01:23 +0200 Subject: [PATCH 05/55] implement fleet store+test --- beater/api/mux.go | 1 - beater/beater.go | 23 +++++++++++- beater/config/config_test.go | 8 +++-- beater/config/rum.go | 3 +- model/error_test.go | 5 ++- model/sourcemap_test.go | 3 +- model/stacktrace_frame_test.go | 6 +++- sourcemap/es_store.go | 13 +++++-- sourcemap/es_store_test.go | 8 ++--- sourcemap/fleet_store.go | 55 ++++++++++++++++++++++++----- sourcemap/fleet_store_test.go | 64 ++++++++++++++++++++++++++++++++++ sourcemap/store.go | 30 +--------------- sourcemap/store_test.go | 13 +++++-- 13 files changed, 174 insertions(+), 58 deletions(-) create mode 100644 sourcemap/fleet_store_test.go diff --git a/beater/api/mux.go b/beater/api/mux.go index 97dd5c8ca0a..2ee9ba6ae47 100644 --- a/beater/api/mux.go +++ b/beater/api/mux.go @@ -163,7 +163,6 @@ func (r *routeBuilder) rumV3IntakeHandler() (request.Handler, error) { } func (r *routeBuilder) sourcemapHandler() (request.Handler, error) { - // TODO: Turn this off, don't allow sourcemap upload in managed mode h := sourcemap.Handler(r.reporter) authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeSourcemapWrite.Action) return middleware.Wrap(h, sourcemapMiddleware(r.cfg, authHandler)...) diff --git a/beater/beater.go b/beater/beater.go index f8ef1002013..b9e9465fe62 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -20,8 +20,10 @@ package beater import ( "context" "net" + "net/http" "regexp" "runtime" + "strings" "sync" "time" @@ -43,6 +45,7 @@ import ( "github.com/elastic/beats/v7/libbeat/publisher/pipetool" "github.com/elastic/apm-server/beater/config" + "github.com/elastic/apm-server/elasticsearch" "github.com/elastic/apm-server/ingest/pipeline" logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/model" @@ -605,7 +608,7 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } if cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled { - store, err := sourcemap.NewStore(beatInfo, cfg.RumConfig.SourceMapping) + store, err := newSourcemapStore(beatInfo, cfg.RumConfig.SourceMapping) if err != nil { return nil, err } @@ -615,6 +618,24 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf return transformConfig, nil } +func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { + logger := logp.NewLogger(logs.Sourcemap) + if len(cfg.SourceMapConfigs) == 0 { + esClient, err := elasticsearch.NewClient(cfg.ESConfig) + if err != nil { + return nil, err + } + index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) + b := sourcemap.NewESStore(esClient, index, logger) + + return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) + } + + b := sourcemap.NewFleetStore(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, http.DefaultClient) + + return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) +} + // WrapRunServerWithProcessors wraps runServer such that it wraps args.Reporter // with a function that event batches are first passed through the given processors // in order. diff --git a/beater/config/config_test.go b/beater/config/config_test.go index eaf40ee5345..3ec518120ea 100644 --- a/beater/config/config_test.go +++ b/beater/config/config_test.go @@ -194,7 +194,8 @@ func TestUnpackConfig(t *testing.T) { MaxRetries: 3, Backoff: elasticsearch.DefaultBackoffConfig, }, - esConfigured: true, + SourceMapConfigs: []SourceMapConfig{}, + esConfigured: true, }, LibraryPattern: "^custom", ExcludeFromGrouping: "^grouping", @@ -363,8 +364,9 @@ func TestUnpackConfig(t *testing.T) { Cache: Cache{ Expiration: 7 * time.Second, }, - IndexPattern: "apm-*-sourcemap*", - ESConfig: elasticsearch.DefaultConfig(), + IndexPattern: "apm-*-sourcemap*", + ESConfig: elasticsearch.DefaultConfig(), + SourceMapConfigs: []SourceMapConfig{}, }, LibraryPattern: "rum", ExcludeFromGrouping: "^/webpack", diff --git a/beater/config/rum.go b/beater/config/rum.go index 124b1a4796b..80be4520233 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -88,7 +88,6 @@ func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg if len(c.SourceMapping.SourceMapConfigs) > 0 { return nil } - c.SourceMapping.ESConfig = elasticsearch.DefaultConfig() var apiKey string if c.SourceMapping.esConfigured { @@ -129,7 +128,7 @@ func defaultSourcemapping() SourceMapping { Enabled: true, Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, IndexPattern: defaultSourcemapIndexPattern, - ESConfig: nil, + ESConfig: elasticsearch.DefaultConfig(), SourceMapConfigs: []SourceMapConfig{}, } } diff --git a/model/error_test.go b/model/error_test.go index dc792f48ef2..008a57b11bc 100644 --- a/model/error_test.go +++ b/model/error_test.go @@ -30,12 +30,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/sourcemap" "github.com/elastic/apm-server/sourcemap/test" "github.com/elastic/apm-server/tests" "github.com/elastic/apm-server/transform" "github.com/elastic/beats/v7/libbeat/common" + "github.com/elastic/beats/v7/libbeat/logp" ) func baseException() *Exception { @@ -805,7 +807,8 @@ func TestSourcemapping(t *testing.T) { assert.Equal(t, 1, *event.Exception.Stacktrace[0].Lineno) // transform with sourcemap store - store, err := sourcemap.NewStore(test.ESClientWithValidSourcemap(t), "apm-*sourcemap*", time.Minute) + b := sourcemap.NewESStore(test.ESClientWithValidSourcemap(t), "apm-*sourcemap*", logp.NewLogger(logs.Sourcemap)) + store, err := sourcemap.NewStore(b, logp.NewLogger(logs.Sourcemap), time.Minute) require.NoError(t, err) transformedWithSourcemap := event.fields(context.Background(), &transform.Config{ RUM: transform.RUMConfig{SourcemapStore: store}, diff --git a/model/sourcemap_test.go b/model/sourcemap_test.go index 2b9e6d20414..452640d0ee0 100644 --- a/model/sourcemap_test.go +++ b/model/sourcemap_test.go @@ -86,7 +86,8 @@ func TestInvalidateCache(t *testing.T) { // create sourcemap store client, err := estest.NewElasticsearchClient(estest.NewTransport(t, http.StatusOK, nil)) require.NoError(t, err) - store, err := sourcemap.NewStore(client, "foo", time.Minute) + b := sourcemap.NewESStore(client, "foo", logp.NewLogger(logs.Sourcemap)) + store, err := sourcemap.NewStore(b, logp.NewLogger(logs.Sourcemap), time.Minute) require.NoError(t, err) // transform with sourcemap store diff --git a/model/stacktrace_frame_test.go b/model/stacktrace_frame_test.go index b3f00cd8e56..e53fcf2f947 100644 --- a/model/stacktrace_frame_test.go +++ b/model/stacktrace_frame_test.go @@ -28,8 +28,10 @@ import ( "github.com/stretchr/testify/require" "github.com/elastic/apm-server/elasticsearch" + logs "github.com/elastic/apm-server/log" "github.com/elastic/beats/v7/libbeat/common" + "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/sourcemap" "github.com/elastic/apm-server/sourcemap/test" @@ -422,7 +424,9 @@ func TestLibraryFrame(t *testing.T) { } func testSourcemapStore(t *testing.T, client elasticsearch.Client) *sourcemap.Store { - store, err := sourcemap.NewStore(client, "apm-*sourcemap*", time.Minute) + logger := logp.NewLogger(logs.Sourcemap) + b := sourcemap.NewESStore(client, "apm-*sourcemap*", logger) + store, err := sourcemap.NewStore(b, logger, time.Minute) require.NoError(t, err) return store } diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index 84afd0ca7c8..99d4c1f4a57 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -44,7 +44,8 @@ var ( errSourcemapWrongFormat = errors.New("Sourcemapping ES Result not in expected format") ) -type esStore struct { +// ESStore handles making sourcemap requests to an ElasticSearch backend. +type ESStore struct { client elasticsearch.Client index string logger *logp.Logger @@ -65,7 +66,13 @@ type esSourcemapResponse struct { } `json:"hits"` } -func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { +// NewESStore returns an instance of ESStore for interacting with sourcemaps +// stored in ElasticSearch. +func NewESStore(c elasticsearch.Client, index string, logger *logp.Logger) *ESStore { + return &ESStore{c, index, logger} +} + +func (s *ESStore) fetch(ctx context.Context, name, version, path string) (string, error) { statusCode, body, err := s.runSearchQuery(ctx, name, version, path) if err != nil { return "", errors.Wrap(err, errMsgESFailure) @@ -87,7 +94,7 @@ func (s *esStore) fetch(ctx context.Context, name, version, path string) (string return parse(body, name, version, path, s.logger) } -func (s *esStore) runSearchQuery(ctx context.Context, name, version, path string) (int, io.ReadCloser, error) { +func (s *ESStore) runSearchQuery(ctx context.Context, name, version, path string) (int, io.ReadCloser, error) { // build and encode the query var buf bytes.Buffer if err := json.NewEncoder(&buf).Encode(query(name, version, path)); err != nil { diff --git a/sourcemap/es_store_test.go b/sourcemap/es_store_test.go index 20a5a313f89..a13349e3c3b 100644 --- a/sourcemap/es_store_test.go +++ b/sourcemap/es_store_test.go @@ -36,7 +36,7 @@ import ( "github.com/elastic/apm-server/sourcemap/test" ) -func Test_esFetcher_fetchError(t *testing.T) { +func Test_ESFetcher_fetchError(t *testing.T) { for name, tc := range map[string]struct { statusCode int esBody map[string]interface{} @@ -77,7 +77,7 @@ func Test_esFetcher_fetchError(t *testing.T) { } } -func Test_esFetcher_fetch(t *testing.T) { +func Test_ESFetcher_fetch(t *testing.T) { for name, tc := range map[string]struct { client elasticsearch.Client filePath string @@ -101,6 +101,6 @@ func Test_esFetcher_fetch(t *testing.T) { } } -func testESStore(client elasticsearch.Client) *esStore { - return &esStore{client: client, index: "apm-sourcemap", logger: logp.NewLogger(logs.Sourcemap)} +func testESStore(client elasticsearch.Client) *ESStore { + return NewESStore(client, "apm-sourcemap", logp.NewLogger(logs.Sourcemap)) } diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 4a819474581..1e45f1d0beb 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -18,23 +18,60 @@ package sourcemap import ( + "bytes" "context" + "fmt" + "io" "net/http" "github.com/elastic/apm-server/beater/config" ) -type fleetStore struct { - apiKey string - c *http.Client - cfgs []config.SourceMapConfig +// FleetStore handles making sourcemap requests to a Fleet-Server. +type FleetStore struct { + apikey string + c *http.Client + fleetURLs map[string]string } -func newFleetBackend(apiKey string, cfgs []config.SourceMapConfig, c *http.Client) fleetStore { - return fleetStore{apiKey: apiKey, cfgs: cfgs, c: c} +// NewFleetStore returns an instance of ESStore for interacting with sourcemaps +// stored in Fleet-Server. +func NewFleetStore(apikey string, cfgs []config.SourceMapConfig, c *http.Client) FleetStore { + fleetURLs := make(map[string]string) + for _, cfg := range cfgs { + k := key([]string{cfg.Service.Name, cfg.Service.Version, cfg.Bundle.Filepath}) + fleetURLs[k] = cfg.SourceMap.URL + } + return FleetStore{ + apikey: "ApiKey " + apikey, + fleetURLs: fleetURLs, + c: c, + } } -func (f fleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { - // find the right sourcemap config, make request with its sourcemap.url - return "", nil +func (f FleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { + k := key([]string{name, version, path}) + fleetURL, ok := f.fleetURLs[k] + if !ok { + return "", fmt.Errorf("unable to find sourcemap.url for service.name=%s service.version=%s bundle.path=%s", + name, version, path, + ) + } + req, err := http.NewRequest(http.MethodGet, fleetURL, nil) + if err != nil { + return "", err + } + req.Header.Add("Authorization", f.apikey) + + resp, err := f.c.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + buf := new(bytes.Buffer) + + io.Copy(buf, resp.Body) + + return buf.String(), nil } diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go new file mode 100644 index 00000000000..6c4313cebd6 --- /dev/null +++ b/sourcemap/fleet_store_test.go @@ -0,0 +1,64 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package sourcemap + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/elastic/apm-server/beater/config" + + "github.com/stretchr/testify/assert" +) + +func TestFetch(t *testing.T) { + var ( + hasAuth bool + apikey = "supersecret" + name = "webapp" + version = "1.0.0" + path = "/my/path/to/bundle.js.map" + wantRes = "sourcemap response" + c = http.DefaultClient + ) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth := r.Header.Get("Authorization") + hasAuth = auth == "ApiKey "+apikey + w.Write([]byte(wantRes)) + })) + defer ts.Close() + + cfgs := []config.SourceMapConfig{ + { + Service: config.Service{Name: name, Version: version}, + Bundle: config.Bundle{Filepath: path}, + SourceMap: config.SourceMap{URL: ts.URL}, + }, + } + fb := NewFleetStore(apikey, cfgs, c) + + gotRes, err := fb.fetch(context.Background(), name, version, path) + assert.NoError(t, err) + + assert.Equal(t, wantRes, gotRes) + + assert.True(t, hasAuth) +} diff --git a/sourcemap/store.go b/sourcemap/store.go index 6cf72e3d578..3d0f4b04ce0 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -20,21 +20,14 @@ package sourcemap import ( "context" "math" - "net/http" "strings" "time" - "github.com/elastic/apm-server/beater/config" - "github.com/elastic/apm-server/elasticsearch" - "github.com/go-sourcemap/sourcemap" gocache "github.com/patrickmn/go-cache" "github.com/pkg/errors" - "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/libbeat/logp" - - logs "github.com/elastic/apm-server/log" ) const ( @@ -59,31 +52,11 @@ type backend interface { // NewStore creates a new instance for fetching sourcemaps based on the // provided config.SourceMapping. Only one of source_mapping.elasticsearch or // sourcemapping.source_maps can be configured. -func NewStore(beatInfo beat.Info, cfg config.SourceMapping) (*Store, error) { - // We're already checking for this in config parsing, so do we need it - // here a second time? - if cfg.ESConfig != nil && len(cfg.SourceMapConfigs) > 0 { - return nil, errors.New("configuring both source_mapping.elasticsearch and sourcemapping.source_maps not allowed") - } - expiration := cfg.Cache.Expiration +func NewStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, error) { if expiration < 0 { return nil, errInit } - var b backend - logger := logp.NewLogger(logs.Sourcemap) - - if len(cfg.SourceMapConfigs) == 0 { - esClient, err := elasticsearch.NewClient(cfg.ESConfig) - if err != nil { - return nil, err - } - index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) - b = &esStore{client: esClient, index: index, logger: logger} - } else { - b = newFleetBackend(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, http.DefaultClient) - } - return &Store{ cache: gocache.New(expiration, cleanupInterval(expiration)), backend: b, @@ -104,7 +77,6 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str // fetch from Elasticsearch and ensure caching for all non-temporary results sourcemapStr, err := s.backend.fetch(ctx, name, version, path) if err != nil { - // TODO: Check for errors from both stores if !strings.Contains(err.Error(), errMsgESFailure) { s.add(key, nil) } diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 6c18722166e..db0ace1db83 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -29,15 +29,19 @@ import ( "github.com/stretchr/testify/require" "github.com/elastic/apm-server/elasticsearch" + logs "github.com/elastic/apm-server/log" + "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/sourcemap/test" ) func Test_NewStore(t *testing.T) { - _, err := NewStore(nil, "", -1) + logger := logp.NewLogger(logs.Sourcemap) + + _, err := NewStore(nil, logger, -1) require.Error(t, err) - f, err := NewStore(nil, "", 100) + f, err := NewStore(nil, logger, 100) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -202,7 +206,10 @@ func TestCleanupInterval(t *testing.T) { } func testStore(t *testing.T, client elasticsearch.Client) *Store { - store, err := NewStore(client, "apm-*sourcemap*", time.Minute) + logger := logp.NewLogger(logs.Sourcemap) + b := NewESStore(client, "apm-*sourcemap*", logger) + + store, err := NewStore(b, logger, time.Minute) require.NoError(t, err) return store } From b612d3f2809d0e5680b39740fb2f7384312e8620 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 3 Jun 2021 17:22:13 +0200 Subject: [PATCH 06/55] [store/fleet] use same Transport as es client Re-use the RoundTripper configuration code for the es client to configure the fleet http client the same way. Internally, the es client is instrumented, so do that as well. --- beater/beater.go | 13 ++++++++++++- elasticsearch/config.go | 5 +++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index b9e9465fe62..b52cc25fda2 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -31,6 +31,7 @@ import ( "github.com/pkg/errors" "go.elastic.co/apm" + "go.elastic.co/apm/module/apmhttp" "golang.org/x/sync/errgroup" "github.com/elastic/beats/v7/libbeat/beat" @@ -631,7 +632,17 @@ func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) } - b := sourcemap.NewFleetStore(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, http.DefaultClient) + // make a copy of the default http client, configure a Transport to be + // the same as the esClient, instrument it, and set it on the http + // client. + c := *http.DefaultClient + tr, err := elasticsearch.NewHTTPTransport(cfg.ESConfig) + if err != nil { + return nil, err + } + + c.Transport = apmhttp.WrapRoundTripper(tr) + b := sourcemap.NewFleetStore(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, &c) return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) } diff --git a/elasticsearch/config.go b/elasticsearch/config.go index 91ff11e822e..24ef3e6b7bf 100644 --- a/elasticsearch/config.go +++ b/elasticsearch/config.go @@ -88,7 +88,7 @@ func connectionConfig(config *Config) (http.RoundTripper, []string, http.Header, if err != nil { return nil, nil, nil, err } - transp, err := httpTransport(config) + transp, err := NewHTTPTransport(config) if err != nil { return nil, nil, nil, err } @@ -134,7 +134,8 @@ func addresses(cfg *Config) ([]string, error) { return addresses, nil } -func httpTransport(cfg *Config) (*http.Transport, error) { +// NewHttpTransport configures an *http.Transport according to a *Config +func NewHTTPTransport(cfg *Config) (*http.Transport, error) { proxy, err := httpProxyURL(cfg) if err != nil { return nil, err From 382e5dd0bbebf343543e9d5dcf81eeaa04b5d720 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 10:21:01 +0200 Subject: [PATCH 07/55] remove old sourcemap api key code this was a temporary fix --- apmpackage/apm/0.3.0/manifest.yml | 6 ------ beater/config/rum.go | 13 ------------- 2 files changed, 19 deletions(-) diff --git a/apmpackage/apm/0.3.0/manifest.yml b/apmpackage/apm/0.3.0/manifest.yml index a9fe2a411ba..618c3c612f3 100644 --- a/apmpackage/apm/0.3.0/manifest.yml +++ b/apmpackage/apm/0.3.0/manifest.yml @@ -120,12 +120,6 @@ policy_templates: required: false show_user: false default: 1000 - - name: sourcemap_api_key - type: text - title: RUM - API Key for Sourcemaps - required: false - description: API Key for sourcemap feature. Enter as : - show_user: false - name: api_key_limit type: integer title: Maximum number of API Keys for Agent authentication diff --git a/beater/config/rum.go b/beater/config/rum.go index 80be4520233..a9c922ba05e 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -89,16 +89,6 @@ func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg return nil } - var apiKey string - if c.SourceMapping.esConfigured { - if dataStreamsEnabled { - // when running under Fleet, the only setting configured is the api key - apiKey = c.SourceMapping.ESConfig.APIKey - } else { - return nil - } - } - // fall back to elasticsearch output configuration for sourcemap storage if possible if outputESCfg == nil { log.Info("Unable to determine sourcemap storage, sourcemaps will not be applied") @@ -108,9 +98,6 @@ func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg if err := outputESCfg.Unpack(c.SourceMapping.ESConfig); err != nil { return errors.Wrap(err, "unpacking Elasticsearch config into Sourcemap config") } - if c.SourceMapping.ESConfig.APIKey == "" { - c.SourceMapping.ESConfig.APIKey = apiKey - } return nil } From d100bc18fc296bed5d345af57fadddba13ed4bcc Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 10:27:43 +0200 Subject: [PATCH 08/55] use `key` struct for fleet config cache --- sourcemap/fleet_store.go | 14 ++++++++++---- sourcemap/store.go | 6 +++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 1e45f1d0beb..12e752912a0 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -31,15 +31,21 @@ import ( type FleetStore struct { apikey string c *http.Client - fleetURLs map[string]string + fleetURLs map[key]string +} + +type key struct { + ServiceName string + ServiceVersion string + BundleFilepath string } // NewFleetStore returns an instance of ESStore for interacting with sourcemaps // stored in Fleet-Server. func NewFleetStore(apikey string, cfgs []config.SourceMapConfig, c *http.Client) FleetStore { - fleetURLs := make(map[string]string) + fleetURLs := make(map[key]string) for _, cfg := range cfgs { - k := key([]string{cfg.Service.Name, cfg.Service.Version, cfg.Bundle.Filepath}) + k := key{cfg.Service.Name, cfg.Service.Version, cfg.Bundle.Filepath} fleetURLs[k] = cfg.SourceMap.URL } return FleetStore{ @@ -50,7 +56,7 @@ func NewFleetStore(apikey string, cfgs []config.SourceMapConfig, c *http.Client) } func (f FleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { - k := key([]string{name, version, path}) + k := key{name, version, path} fleetURL, ok := f.fleetURLs[k] if !ok { return "", fmt.Errorf("unable to find sourcemap.url for service.name=%s service.version=%s bundle.path=%s", diff --git a/sourcemap/store.go b/sourcemap/store.go index 3d0f4b04ce0..b6c01fc579f 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -66,7 +66,7 @@ func NewStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, // Fetch a sourcemap from the store. func (s *Store) Fetch(ctx context.Context, name string, version string, path string) (*sourcemap.Consumer, error) { - key := key([]string{name, version, path}) + key := cacheKey([]string{name, version, path}) // fetch from cache if val, found := s.cache.Get(key); found { @@ -103,7 +103,7 @@ func (s *Store) Added(ctx context.Context, name string, version string, path str s.logger.Warnf("Overriding sourcemap for service %s version %s and file %s", name, version, path) } - key := key([]string{name, version, path}) + key := cacheKey([]string{name, version, path}) s.cache.Delete(key) if !s.logger.IsDebug() { return @@ -119,7 +119,7 @@ func (s *Store) add(key string, consumer *sourcemap.Consumer) { s.logger.Debugf("Added id %v. Cache now has %v entries.", key, s.cache.ItemCount()) } -func key(s []string) string { +func cacheKey(s []string) string { return strings.Join(s, "_") } From 3ccda4c2f162162f4e9953f11d249b3431b81614 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 11:01:28 +0200 Subject: [PATCH 09/55] flatten sourcemap config struct defn --- beater/config/agentconfig.go | 4 ---- beater/config/config_test.go | 21 ++++++++++++++++++--- beater/config/sourcemapping.go | 16 +++++----------- sourcemap/fleet_store.go | 4 ++-- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/beater/config/agentconfig.go b/beater/config/agentconfig.go index fb7e9751773..22be77da0c4 100644 --- a/beater/config/agentconfig.go +++ b/beater/config/agentconfig.go @@ -71,7 +71,6 @@ func (s *AgentConfig) setup() error { type Service struct { Name string `config:"name"` Environment string `config:"environment"` - Version string `config:"version"` } // String implements the Stringer interface. @@ -83,8 +82,5 @@ func (s *Service) String() string { if s.Environment != "" { env = "service.environment=" + s.Environment } - if s.Version != "" { - version = "service.version=" + s.Version - } return strings.Join([]string{name, env, version}, " ") } diff --git a/beater/config/config_test.go b/beater/config/config_test.go index 3ec518120ea..f6e7af23bef 100644 --- a/beater/config/config_test.go +++ b/beater/config/config_test.go @@ -293,6 +293,14 @@ func TestUnpackConfig(t *testing.T) { "rum": map[string]interface{}{ "enabled": true, "source_mapping": map[string]interface{}{ + "source_maps": []map[string]string{ + { + "service.name": "opbeans-rum", + "service.version": "1.2.3", + "bundle.filepath": "/test/e2e/general-usecase/bundle.js.map", + "sourcemap.url": "http://somewhere.com/bundle.js.map", + }, + }, "cache": map[string]interface{}{ "expiration": 7, }, @@ -364,9 +372,16 @@ func TestUnpackConfig(t *testing.T) { Cache: Cache{ Expiration: 7 * time.Second, }, - IndexPattern: "apm-*-sourcemap*", - ESConfig: elasticsearch.DefaultConfig(), - SourceMapConfigs: []SourceMapConfig{}, + IndexPattern: "apm-*-sourcemap*", + ESConfig: elasticsearch.DefaultConfig(), + SourceMapConfigs: []SourceMapConfig{ + { + ServiceName: "opbeans-rum", + ServiceVersion: "1.2.3", + BundleFilepath: "/test/e2e/general-usecase/bundle.js.map", + SourceMapURL: "http://somewhere.com/bundle.js.map", + }, + }, }, LibraryPattern: "rum", ExcludeFromGrouping: "^/webpack", diff --git a/beater/config/sourcemapping.go b/beater/config/sourcemapping.go index f87b4b07d26..d14c51116e0 100644 --- a/beater/config/sourcemapping.go +++ b/beater/config/sourcemapping.go @@ -17,16 +17,10 @@ package config +// SourceMapConfig holds source map configuration information. type SourceMapConfig struct { - Service Service `config:"service"` - Bundle Bundle `config:"bundle"` - SourceMap SourceMap `config:"sourcemap"` -} - -type Bundle struct { - Filepath string `config:"filepath"` -} - -type SourceMap struct { - URL string `config:"url"` + ServiceName string `config:"service.name"` + ServiceVersion string `config:"service.version"` + BundleFilepath string `config:"bundle.filepath"` + SourceMapURL string `config:"sourcemap.url"` } diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 12e752912a0..75de04b4421 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -45,8 +45,8 @@ type key struct { func NewFleetStore(apikey string, cfgs []config.SourceMapConfig, c *http.Client) FleetStore { fleetURLs := make(map[key]string) for _, cfg := range cfgs { - k := key{cfg.Service.Name, cfg.Service.Version, cfg.Bundle.Filepath} - fleetURLs[k] = cfg.SourceMap.URL + k := key{cfg.ServiceName, cfg.ServiceVersion, cfg.BundleFilepath} + fleetURLs[k] = cfg.SourceMapURL } return FleetStore{ apikey: "ApiKey " + apikey, From 679f2b5f8aca44e8b09fa94f435f73245c4b869f Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 11:44:50 +0200 Subject: [PATCH 10/55] public elasticsearch/fleet constructor methods --- beater/beater.go | 11 +++-------- model/error_test.go | 5 +---- model/sourcemap_test.go | 3 +-- model/stacktrace_frame_test.go | 6 +----- sourcemap/es_store.go | 24 ++++++++++++++++-------- sourcemap/es_store_test.go | 8 ++++---- sourcemap/fleet_store.go | 26 ++++++++++++++++++++------ sourcemap/fleet_store_test.go | 9 +++++---- sourcemap/store.go | 5 +---- sourcemap/store_test.go | 11 ++++------- 10 files changed, 56 insertions(+), 52 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index b52cc25fda2..02ff2062764 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -620,16 +620,13 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { - logger := logp.NewLogger(logs.Sourcemap) if len(cfg.SourceMapConfigs) == 0 { - esClient, err := elasticsearch.NewClient(cfg.ESConfig) + c, err := elasticsearch.NewClient(cfg.ESConfig) if err != nil { return nil, err } index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) - b := sourcemap.NewESStore(esClient, index, logger) - - return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) + return sourcemap.NewElasticsearchStore(c, index, cfg.Cache.Expiration) } // make a copy of the default http client, configure a Transport to be @@ -642,9 +639,7 @@ func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap } c.Transport = apmhttp.WrapRoundTripper(tr) - b := sourcemap.NewFleetStore(cfg.ESConfig.APIKey, cfg.SourceMapConfigs, &c) - - return sourcemap.NewStore(b, logger, cfg.Cache.Expiration) + return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.SourceMapConfigs, cfg.Cache.Expiration) } // WrapRunServerWithProcessors wraps runServer such that it wraps args.Reporter diff --git a/model/error_test.go b/model/error_test.go index 008a57b11bc..d1af07d44b3 100644 --- a/model/error_test.go +++ b/model/error_test.go @@ -30,14 +30,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/sourcemap" "github.com/elastic/apm-server/sourcemap/test" "github.com/elastic/apm-server/tests" "github.com/elastic/apm-server/transform" "github.com/elastic/beats/v7/libbeat/common" - "github.com/elastic/beats/v7/libbeat/logp" ) func baseException() *Exception { @@ -807,8 +805,7 @@ func TestSourcemapping(t *testing.T) { assert.Equal(t, 1, *event.Exception.Stacktrace[0].Lineno) // transform with sourcemap store - b := sourcemap.NewESStore(test.ESClientWithValidSourcemap(t), "apm-*sourcemap*", logp.NewLogger(logs.Sourcemap)) - store, err := sourcemap.NewStore(b, logp.NewLogger(logs.Sourcemap), time.Minute) + store, err := sourcemap.NewElasticsearchStore(test.ESClientWithValidSourcemap(t), "apm-*sourcemap*", time.Minute) require.NoError(t, err) transformedWithSourcemap := event.fields(context.Background(), &transform.Config{ RUM: transform.RUMConfig{SourcemapStore: store}, diff --git a/model/sourcemap_test.go b/model/sourcemap_test.go index 452640d0ee0..6ddec9ec273 100644 --- a/model/sourcemap_test.go +++ b/model/sourcemap_test.go @@ -86,8 +86,7 @@ func TestInvalidateCache(t *testing.T) { // create sourcemap store client, err := estest.NewElasticsearchClient(estest.NewTransport(t, http.StatusOK, nil)) require.NoError(t, err) - b := sourcemap.NewESStore(client, "foo", logp.NewLogger(logs.Sourcemap)) - store, err := sourcemap.NewStore(b, logp.NewLogger(logs.Sourcemap), time.Minute) + store, err := sourcemap.NewElasticsearchStore(client, "foo", time.Minute) require.NoError(t, err) // transform with sourcemap store diff --git a/model/stacktrace_frame_test.go b/model/stacktrace_frame_test.go index e53fcf2f947..8c890b8afbb 100644 --- a/model/stacktrace_frame_test.go +++ b/model/stacktrace_frame_test.go @@ -28,10 +28,8 @@ import ( "github.com/stretchr/testify/require" "github.com/elastic/apm-server/elasticsearch" - logs "github.com/elastic/apm-server/log" "github.com/elastic/beats/v7/libbeat/common" - "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/sourcemap" "github.com/elastic/apm-server/sourcemap/test" @@ -424,9 +422,7 @@ func TestLibraryFrame(t *testing.T) { } func testSourcemapStore(t *testing.T, client elasticsearch.Client) *sourcemap.Store { - logger := logp.NewLogger(logs.Sourcemap) - b := sourcemap.NewESStore(client, "apm-*sourcemap*", logger) - store, err := sourcemap.NewStore(b, logger, time.Minute) + store, err := sourcemap.NewElasticsearchStore(client, "apm-*sourcemap*", time.Minute) require.NoError(t, err) return store } diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index 99d4c1f4a57..ad999f4d17e 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -25,12 +25,14 @@ import ( "io" "io/ioutil" "net/http" + "time" "github.com/pkg/errors" "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/elasticsearch" + logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/utility" ) @@ -44,8 +46,7 @@ var ( errSourcemapWrongFormat = errors.New("Sourcemapping ES Result not in expected format") ) -// ESStore handles making sourcemap requests to an ElasticSearch backend. -type ESStore struct { +type esStore struct { client elasticsearch.Client index string logger *logp.Logger @@ -66,13 +67,20 @@ type esSourcemapResponse struct { } `json:"hits"` } -// NewESStore returns an instance of ESStore for interacting with sourcemaps -// stored in ElasticSearch. -func NewESStore(c elasticsearch.Client, index string, logger *logp.Logger) *ESStore { - return &ESStore{c, index, logger} +// NewElasticsearchStore returns an instance of Store for interacting with +// sourcemaps stored in ElasticSearch. +func NewElasticsearchStore( + c elasticsearch.Client, + index string, + expiration time.Duration, +) (*Store, error) { + logger := logp.NewLogger(logs.Sourcemap) + s := &esStore{c, index, logger} + + return newStore(s, logger, expiration) } -func (s *ESStore) fetch(ctx context.Context, name, version, path string) (string, error) { +func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { statusCode, body, err := s.runSearchQuery(ctx, name, version, path) if err != nil { return "", errors.Wrap(err, errMsgESFailure) @@ -94,7 +102,7 @@ func (s *ESStore) fetch(ctx context.Context, name, version, path string) (string return parse(body, name, version, path, s.logger) } -func (s *ESStore) runSearchQuery(ctx context.Context, name, version, path string) (int, io.ReadCloser, error) { +func (s *esStore) runSearchQuery(ctx context.Context, name, version, path string) (int, io.ReadCloser, error) { // build and encode the query var buf bytes.Buffer if err := json.NewEncoder(&buf).Encode(query(name, version, path)); err != nil { diff --git a/sourcemap/es_store_test.go b/sourcemap/es_store_test.go index a13349e3c3b..20a5a313f89 100644 --- a/sourcemap/es_store_test.go +++ b/sourcemap/es_store_test.go @@ -36,7 +36,7 @@ import ( "github.com/elastic/apm-server/sourcemap/test" ) -func Test_ESFetcher_fetchError(t *testing.T) { +func Test_esFetcher_fetchError(t *testing.T) { for name, tc := range map[string]struct { statusCode int esBody map[string]interface{} @@ -77,7 +77,7 @@ func Test_ESFetcher_fetchError(t *testing.T) { } } -func Test_ESFetcher_fetch(t *testing.T) { +func Test_esFetcher_fetch(t *testing.T) { for name, tc := range map[string]struct { client elasticsearch.Client filePath string @@ -101,6 +101,6 @@ func Test_ESFetcher_fetch(t *testing.T) { } } -func testESStore(client elasticsearch.Client) *ESStore { - return NewESStore(client, "apm-sourcemap", logp.NewLogger(logs.Sourcemap)) +func testESStore(client elasticsearch.Client) *esStore { + return &esStore{client: client, index: "apm-sourcemap", logger: logp.NewLogger(logs.Sourcemap)} } diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 75de04b4421..772d9a855d4 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -23,12 +23,15 @@ import ( "fmt" "io" "net/http" + "time" + + "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/beater/config" + logs "github.com/elastic/apm-server/log" ) -// FleetStore handles making sourcemap requests to a Fleet-Server. -type FleetStore struct { +type fleetStore struct { apikey string c *http.Client fleetURLs map[key]string @@ -40,22 +43,33 @@ type key struct { BundleFilepath string } -// NewFleetStore returns an instance of ESStore for interacting with sourcemaps +// NewFleetStore returns an instance of Store for interacting with sourcemaps // stored in Fleet-Server. -func NewFleetStore(apikey string, cfgs []config.SourceMapConfig, c *http.Client) FleetStore { +func NewFleetStore( + c *http.Client, + apikey string, + cfgs []config.SourceMapConfig, + expiration time.Duration, +) (*Store, error) { + logger := logp.NewLogger(logs.Sourcemap) + s := newFleetStore(c, apikey, cfgs) + return newStore(s, logger, expiration) +} + +func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapConfig) fleetStore { fleetURLs := make(map[key]string) for _, cfg := range cfgs { k := key{cfg.ServiceName, cfg.ServiceVersion, cfg.BundleFilepath} fleetURLs[k] = cfg.SourceMapURL } - return FleetStore{ + return fleetStore{ apikey: "ApiKey " + apikey, fleetURLs: fleetURLs, c: c, } } -func (f FleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { +func (f fleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { k := key{name, version, path} fleetURL, ok := f.fleetURLs[k] if !ok { diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go index 6c4313cebd6..ce386c21e63 100644 --- a/sourcemap/fleet_store_test.go +++ b/sourcemap/fleet_store_test.go @@ -48,12 +48,13 @@ func TestFetch(t *testing.T) { cfgs := []config.SourceMapConfig{ { - Service: config.Service{Name: name, Version: version}, - Bundle: config.Bundle{Filepath: path}, - SourceMap: config.SourceMap{URL: ts.URL}, + ServiceName: name, + ServiceVersion: version, + BundleFilepath: path, + SourceMapURL: ts.URL, }, } - fb := NewFleetStore(apikey, cfgs, c) + fb := newFleetStore(c, apikey, cfgs) gotRes, err := fb.fetch(context.Background(), name, version, path) assert.NoError(t, err) diff --git a/sourcemap/store.go b/sourcemap/store.go index b6c01fc579f..f2bd8cf08e8 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -49,10 +49,7 @@ type backend interface { fetch(ctx context.Context, name, version, path string) (string, error) } -// NewStore creates a new instance for fetching sourcemaps based on the -// provided config.SourceMapping. Only one of source_mapping.elasticsearch or -// sourcemapping.source_maps can be configured. -func NewStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, error) { +func newStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, error) { if expiration < 0 { return nil, errInit } diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index db0ace1db83..64ecd20242d 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -35,13 +35,13 @@ import ( "github.com/elastic/apm-server/sourcemap/test" ) -func Test_NewStore(t *testing.T) { +func Test_newStore(t *testing.T) { logger := logp.NewLogger(logs.Sourcemap) - _, err := NewStore(nil, logger, -1) + _, err := newStore(nil, logger, -1) require.Error(t, err) - f, err := NewStore(nil, logger, 100) + f, err := newStore(nil, logger, 100) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -206,10 +206,7 @@ func TestCleanupInterval(t *testing.T) { } func testStore(t *testing.T, client elasticsearch.Client) *Store { - logger := logp.NewLogger(logs.Sourcemap) - b := NewESStore(client, "apm-*sourcemap*", logger) - - store, err := NewStore(b, logger, time.Minute) + store, err := NewElasticsearchStore(client, "apm-*sourcemap*", time.Minute) require.NoError(t, err) return store } From c3a8aa82695e3042d26fb0fbe91917a2b1e5c752 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 14:56:39 +0200 Subject: [PATCH 11/55] synchronize cache fetch on miss if a series of requests come in at once for a key that isn't in the cache, coordinate a single request to the backend to retrieve it and have the others wait until the request is complete. --- sourcemap/fleet_store.go | 10 ++++++ sourcemap/store.go | 43 +++++++++++++++++++++--- sourcemap/store_test.go | 72 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 772d9a855d4..f49084e1c99 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -22,6 +22,7 @@ import ( "context" "fmt" "io" + "io/ioutil" "net/http" "time" @@ -89,6 +90,15 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri } defer resp.Body.Close() + // Verify that we should only get 200 back from fleet-server + if resp.StatusCode != http.StatusOK { + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("failure querying fleet: statuscode=%d response=(failed to read body)", resp.StatusCode) + } + return "", fmt.Errorf("failure querying fleet: statuscode=%d response=%s", resp.StatusCode, body) + } + buf := new(bytes.Buffer) io.Copy(buf, resp.Body) diff --git a/sourcemap/store.go b/sourcemap/store.go index f2bd8cf08e8..f9d2c38cdbf 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -21,6 +21,7 @@ import ( "context" "math" "strings" + "sync" "time" "github.com/go-sourcemap/sourcemap" @@ -43,6 +44,9 @@ type Store struct { cache *gocache.Cache backend backend logger *logp.Logger + + sync.Mutex + inflight map[string]chan struct{} } type backend interface { @@ -55,9 +59,10 @@ func newStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, } return &Store{ - cache: gocache.New(expiration, cleanupInterval(expiration)), - backend: b, - logger: logger, + cache: gocache.New(expiration, cleanupInterval(expiration)), + backend: b, + logger: logger, + inflight: make(map[string]chan struct{}), }, nil } @@ -71,10 +76,39 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str return consumer, nil } + // if the value hasn't been found, check to see if there's an inflight + // request to update the value. + s.Lock() + wait, ok := s.inflight[key] + if ok { + // found an inflight request, wait for it to complete. + s.Unlock() + <-wait + // Try to read the value again + // TODO: Prevent this from being an infinite loop + // Maybe just try s.cache.Get() one more time? + return s.Fetch(ctx, name, version, path) + } else { + // no inflight request found, add a channel to the map and then + // make the fetch request. + wait = make(chan struct{}) + s.inflight[key] = wait + } + s.Unlock() + + // Once the fetch request is complete, close and remove the channel + // from the syncronization map. + defer func() { + s.Lock() + delete(s.inflight, key) + close(wait) + s.Unlock() + }() + // fetch from Elasticsearch and ensure caching for all non-temporary results sourcemapStr, err := s.backend.fetch(ctx, name, version, path) if err != nil { - if !strings.Contains(err.Error(), errMsgESFailure) { + if !strings.Contains(err.Error(), "failure querying") { s.add(key, nil) } return nil, err @@ -91,6 +125,7 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str return nil, errors.Wrap(err, errMsgParseSourcemap) } s.add(key, consumer) + return consumer, nil } diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 64ecd20242d..b43be244cbb 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -20,6 +20,10 @@ package sourcemap import ( "context" "fmt" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" "testing" "time" @@ -28,6 +32,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/elasticsearch" logs "github.com/elastic/apm-server/log" "github.com/elastic/beats/v7/libbeat/logp" @@ -146,6 +151,73 @@ func TestStore_Fetch(t *testing.T) { }) } +func TestConcurrentFetch(t *testing.T) { + for _, tc := range []struct { + calledWant, errWant, succsWant int64 + }{ + {calledWant: 1, errWant: 0, succsWant: 10}, + {calledWant: 2, errWant: 1, succsWant: 9}, + {calledWant: 4, errWant: 3, succsWant: 7}, + } { + var ( + called, errs, succs int64 + + apikey = "supersecret" + name = "webapp" + version = "1.0.0" + path = "/my/path/to/bundle.js.map" + c = http.DefaultClient + + errsLeft = tc.errWant + ) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&called, 1) + // Simulate the wait for a network request. + time.Sleep(50 * time.Millisecond) + if errsLeft > 0 { + errsLeft-- + http.Error(w, "err", http.StatusInternalServerError) + return + } + w.Write([]byte(test.ValidSourcemap)) + })) + defer ts.Close() + + cfgs := []config.SourceMapConfig{ + { + ServiceName: name, + ServiceVersion: version, + BundleFilepath: path, + SourceMapURL: ts.URL, + }, + } + store, err := NewFleetStore(c, apikey, cfgs, time.Minute) + assert.NoError(t, err) + + var wg sync.WaitGroup + for i := 0; i < int(tc.succsWant+tc.errWant); i++ { + wg.Add(1) + go func() { + consumer, err := store.Fetch(context.Background(), name, version, path) + if err != nil { + atomic.AddInt64(&errs, 1) + } else { + assert.NotNil(t, consumer) + atomic.AddInt64(&succs, 1) + } + + wg.Done() + }() + } + + wg.Wait() + assert.Equal(t, tc.errWant, errs) + assert.Equal(t, tc.calledWant, called) + assert.Equal(t, tc.succsWant, succs) + } +} + func TestStore_Added(t *testing.T) { name, version, path := "foo", "1.0.1", "/tmp" key := "foo_1.0.1_/tmp" From c082b7019b2782bc0285e6900a878e45c5fd67a0 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Fri, 4 Jun 2021 15:04:16 +0200 Subject: [PATCH 12/55] write TODO about killing piling up requests --- sourcemap/store.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index f9d2c38cdbf..7d45191d578 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -85,8 +85,10 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str s.Unlock() <-wait // Try to read the value again - // TODO: Prevent this from being an infinite loop - // Maybe just try s.cache.Get() one more time? + // TODO: All waiting Fetch()'s will attempt to re-fetch once + // the coordinated backend request has finished. At some point + // we need to start failing incoming requests as more start + // piling up, if eg. the backend is down. return s.Fetch(ctx, name, version, path) } else { // no inflight request found, add a channel to the map and then From c4d2b1f87d4aec3a3ae39aadb1151ef422ddc26e Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 11:15:08 +0200 Subject: [PATCH 13/55] Add waiters semaphore Limit the number of open requests that are waiting for a response from the sourcemap backend --- sourcemap/es_store.go | 2 +- sourcemap/fleet_store.go | 2 +- sourcemap/store.go | 26 +++++++++----- sourcemap/store_test.go | 76 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 94 insertions(+), 12 deletions(-) diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index ad999f4d17e..ab5dec886c9 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -77,7 +77,7 @@ func NewElasticsearchStore( logger := logp.NewLogger(logs.Sourcemap) s := &esStore{c, index, logger} - return newStore(s, logger, expiration) + return newStore(s, logger, expiration, 25) } func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index f49084e1c99..3ab9e27aecb 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -54,7 +54,7 @@ func NewFleetStore( ) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) s := newFleetStore(c, apikey, cfgs) - return newStore(s, logger, expiration) + return newStore(s, logger, expiration, 25) } func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapConfig) fleetStore { diff --git a/sourcemap/store.go b/sourcemap/store.go index 7d45191d578..6a6752e845a 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -44,6 +44,7 @@ type Store struct { cache *gocache.Cache backend backend logger *logp.Logger + waiters chan struct{} sync.Mutex inflight map[string]chan struct{} @@ -53,15 +54,19 @@ type backend interface { fetch(ctx context.Context, name, version, path string) (string, error) } -func newStore(b backend, logger *logp.Logger, expiration time.Duration) (*Store, error) { +func newStore(b backend, logger *logp.Logger, expiration time.Duration, maxWaiters int) (*Store, error) { if expiration < 0 { return nil, errInit } return &Store{ - cache: gocache.New(expiration, cleanupInterval(expiration)), - backend: b, - logger: logger, + cache: gocache.New(expiration, cleanupInterval(expiration)), + backend: b, + logger: logger, + // TODO: Number of requests waiting for a sourcemap to be + // fetched. Any suggestion as to where we should set this is + // appreciated. + waiters: make(chan struct{}, maxWaiters), inflight: make(map[string]chan struct{}), }, nil } @@ -83,12 +88,17 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str if ok { // found an inflight request, wait for it to complete. s.Unlock() + // Check to see if we should wait for a response + select { + case s.waiters <- struct{}{}: + default: + // waiters is currently full, abort the request. + return nil, errors.New("sourcemap fetch: too many open requests") + } <-wait + // Release back to waiters semaphore + <-s.waiters // Try to read the value again - // TODO: All waiting Fetch()'s will attempt to re-fetch once - // the coordinated backend request has finished. At some point - // we need to start failing incoming requests as more start - // piling up, if eg. the backend is down. return s.Fetch(ctx, name, version, path) } else { // no inflight request found, add a channel to the map and then diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index b43be244cbb..39faa7e6e47 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -43,10 +43,10 @@ import ( func Test_newStore(t *testing.T) { logger := logp.NewLogger(logs.Sourcemap) - _, err := newStore(nil, logger, -1) + _, err := newStore(nil, logger, -1, 25) require.Error(t, err) - f, err := newStore(nil, logger, 100) + f, err := newStore(nil, logger, 100, 25) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -151,6 +151,78 @@ func TestStore_Fetch(t *testing.T) { }) } +func TestWaitersLimit(t *testing.T) { + var ( + errs int64 + + apikey = "supersecret" + name = "webapp" + version = "1.0.0" + path = "/my/path/to/bundle.js.map" + c = http.DefaultClient + ) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(50 * time.Millisecond) + w.Write([]byte(test.ValidSourcemap)) + })) + defer ts.Close() + + cfgs := []config.SourceMapConfig{ + { + ServiceName: name, + ServiceVersion: version, + BundleFilepath: path, + SourceMapURL: ts.URL, + }, + { + ServiceName: name, + ServiceVersion: "1.1.0", + BundleFilepath: path, + SourceMapURL: ts.URL, + }, + } + store, err := NewFleetStore(c, apikey, cfgs, time.Minute) + assert.NoError(t, err) + + var wg sync.WaitGroup + // NewFleetStore is currently limited to 1 active + 25 waiting = 26 + // requests, so the 27th should fail. All others wait and receive a + // successful response. + for i := 0; i < 27; i++ { + wg.Add(1) + go func() { + consumer, err := store.Fetch(context.Background(), name, version, path) + if err != nil { + assert.Nil(t, consumer) + atomic.AddInt64(&errs, 1) + } else { + assert.NotNil(t, consumer) + } + + wg.Done() + }() + } + + wg.Wait() + + assert.Equal(t, int64(1), errs) + + // Verify that the semaphore was released and requests for a different + // key work as expected. + for i := 0; i < 26; i++ { + wg.Add(1) + go func() { + consumer, err := store.Fetch(context.Background(), name, "1.1.0", path) + assert.NoError(t, err) + assert.NotNil(t, consumer) + + wg.Done() + }() + } + + wg.Wait() +} + func TestConcurrentFetch(t *testing.T) { for _, tc := range []struct { calledWant, errWant, succsWant int64 From 22dd903253379fb71cff3ef28f80ebf415be199d Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 11:16:59 +0200 Subject: [PATCH 14/55] remove unnecessary else block --- sourcemap/store.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index 6a6752e845a..e1d498a2554 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -100,12 +100,13 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str <-s.waiters // Try to read the value again return s.Fetch(ctx, name, version, path) - } else { - // no inflight request found, add a channel to the map and then - // make the fetch request. - wait = make(chan struct{}) - s.inflight[key] = wait } + + // no inflight request found, add a channel to the map and then + // make the fetch request. + wait = make(chan struct{}) + s.inflight[key] = wait + s.Unlock() // Once the fetch request is complete, close and remove the channel From c42479e891048b156cd368e59c92a6312a089b4e Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 11:49:39 +0200 Subject: [PATCH 15/55] add timeout for waiting --- sourcemap/es_store.go | 2 +- sourcemap/fleet_store.go | 2 +- sourcemap/store.go | 34 +++++++++++++++++-------- sourcemap/store_test.go | 55 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index ab5dec886c9..0958e923207 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -77,7 +77,7 @@ func NewElasticsearchStore( logger := logp.NewLogger(logs.Sourcemap) s := &esStore{c, index, logger} - return newStore(s, logger, expiration, 25) + return newStore(s, logger, expiration, 10*time.Second, 25) } func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 3ab9e27aecb..d08b84f24f1 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -54,7 +54,7 @@ func NewFleetStore( ) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) s := newFleetStore(c, apikey, cfgs) - return newStore(s, logger, expiration, 25) + return newStore(s, logger, expiration, 10*time.Second, 25) } func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapConfig) fleetStore { diff --git a/sourcemap/store.go b/sourcemap/store.go index e1d498a2554..d5101a32e50 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -41,10 +41,11 @@ var ( // Store holds information necessary to fetch a sourcemap, either from an Elasticsearch instance or an internal cache. type Store struct { - cache *gocache.Cache - backend backend - logger *logp.Logger - waiters chan struct{} + cache *gocache.Cache + backend backend + logger *logp.Logger + waiters chan struct{} + waitTimeout time.Duration sync.Mutex inflight map[string]chan struct{} @@ -54,20 +55,26 @@ type backend interface { fetch(ctx context.Context, name, version, path string) (string, error) } -func newStore(b backend, logger *logp.Logger, expiration time.Duration, maxWaiters int) (*Store, error) { - if expiration < 0 { +func newStore( + b backend, + logger *logp.Logger, + cacheExpiration, waitTimeout time.Duration, + maxWaiters int, +) (*Store, error) { + if cacheExpiration < 0 { return nil, errInit } return &Store{ - cache: gocache.New(expiration, cleanupInterval(expiration)), + cache: gocache.New(cacheExpiration, cleanupInterval(cacheExpiration)), backend: b, logger: logger, // TODO: Number of requests waiting for a sourcemap to be // fetched. Any suggestion as to where we should set this is // appreciated. - waiters: make(chan struct{}, maxWaiters), - inflight: make(map[string]chan struct{}), + waiters: make(chan struct{}, maxWaiters), + waitTimeout: waitTimeout, + inflight: make(map[string]chan struct{}), }, nil } @@ -95,7 +102,14 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str // waiters is currently full, abort the request. return nil, errors.New("sourcemap fetch: too many open requests") } - <-wait + + t := time.NewTimer(s.waitTimeout) + select { + case <-wait: + t.Stop() + case <-t.C: + return nil, errors.New("sourcemap fetch: timeout") + } // Release back to waiters semaphore <-s.waiters // Try to read the value again diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 39faa7e6e47..fd9ed9ded3d 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -43,10 +43,10 @@ import ( func Test_newStore(t *testing.T) { logger := logp.NewLogger(logs.Sourcemap) - _, err := newStore(nil, logger, -1, 25) + _, err := newStore(nil, logger, -1, time.Second, 25) require.Error(t, err) - f, err := newStore(nil, logger, 100, 25) + f, err := newStore(nil, logger, 100, time.Second, 25) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -223,6 +223,57 @@ func TestWaitersLimit(t *testing.T) { wg.Wait() } +func TestWaitersTimeout(t *testing.T) { + var ( + errs int64 + + apikey = "supersecret" + name = "webapp" + version = "1.0.0" + path = "/my/path/to/bundle.js.map" + c = http.DefaultClient + waitc = make(chan struct{}) + ) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-waitc + w.Write([]byte(test.ValidSourcemap)) + })) + defer ts.Close() + + cfgs := []config.SourceMapConfig{ + { + ServiceName: name, + ServiceVersion: version, + BundleFilepath: path, + SourceMapURL: ts.URL, + }, + } + b := newFleetStore(c, apikey, cfgs) + logger := logp.NewLogger(logs.Sourcemap) + store, err := newStore(b, logger, time.Minute, time.Millisecond, 25) + assert.NoError(t, err) + + var wg sync.WaitGroup + for i := 0; i < 2; i++ { + wg.Add(1) + go func() { + consumer, err := store.Fetch(context.Background(), name, version, path) + if err != nil { + assert.Contains(t, err.Error(), "timeout") + atomic.AddInt64(&errs, 1) + close(waitc) + } else { + assert.NotNil(t, consumer) + } + + wg.Done() + }() + } + + wg.Wait() + assert.Equal(t, int64(1), errs) +} + func TestConcurrentFetch(t *testing.T) { for _, tc := range []struct { calledWant, errWant, succsWant int64 From 9bd9bfeff7c9f6b9eb648bbb0899798bd73a8bb2 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 11:51:40 +0200 Subject: [PATCH 16/55] update docs --- _meta/beat.yml | 2 +- apm-server.docker.yml | 2 +- apm-server.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/_meta/beat.yml b/_meta/beat.yml index 82ad0cc8a4d..3db5b899646 100644 --- a/_meta/beat.yml +++ b/_meta/beat.yml @@ -248,7 +248,7 @@ apm-server: # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. - # Note: Configuring elasticsearch is not allowed if apm-server is being + # Note: Configuring elasticsearch is not supported if apm-server is being # managed by Fleet. #elasticsearch: # Array of hosts to connect to. diff --git a/apm-server.docker.yml b/apm-server.docker.yml index af8bdde46d7..07adfb443ce 100644 --- a/apm-server.docker.yml +++ b/apm-server.docker.yml @@ -248,7 +248,7 @@ apm-server: # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. - # Note: Configuring elasticsearch is not allowed if apm-server is being + # Note: Configuring elasticsearch is not supported if apm-server is being # managed by Fleet. #elasticsearch: # Array of hosts to connect to. diff --git a/apm-server.yml b/apm-server.yml index e20126132bd..ee9204e7f39 100644 --- a/apm-server.yml +++ b/apm-server.yml @@ -248,7 +248,7 @@ apm-server: # output.elasticsearch configuration. # A different instance must be configured when using any other output. # This setting only affects sourcemap reads - the output determines where sourcemaps are written. - # Note: Configuring elasticsearch is not allowed if apm-server is being + # Note: Configuring elasticsearch is not supported if apm-server is being # managed by Fleet. #elasticsearch: # Array of hosts to connect to. From f112d11fe336a74c41fb7e087270dd3b7cea520d Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 12:19:29 +0200 Subject: [PATCH 17/55] update changelog --- changelogs/head.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/head.asciidoc b/changelogs/head.asciidoc index 892b690ebd0..2cc30de2151 100644 --- a/changelogs/head.asciidoc +++ b/changelogs/head.asciidoc @@ -25,7 +25,7 @@ https://github.com/elastic/apm-server/compare/7.13\...master[View commits] * Add support for histograms to metrics intake {pull}5360[5360] * Upgrade Go to 1.16.4 {pull}5381[5381] * Add units to metric fields {pull}5395[5395] - +* Support fetching sourcemaps from fleet {pull}5410[5410] [float] ==== Deprecated From 37e0d4afd17f4c93571c87386b9d6055838578ad Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 7 Jun 2021 12:33:32 +0200 Subject: [PATCH 18/55] stop configuring fleet-server http client we won't be re-using elasticsearch config for fleet-server configuration. --- beater/beater.go | 12 ++++-------- elasticsearch/config.go | 5 ++--- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index 02ff2062764..1c22a4b92a3 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -629,16 +629,12 @@ func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap return sourcemap.NewElasticsearchStore(c, index, cfg.Cache.Expiration) } - // make a copy of the default http client, configure a Transport to be - // the same as the esClient, instrument it, and set it on the http - // client. + // TODO: Configure the client with fleet TLS certs, timeouts, etc. + // Fleet hands TLS certs to the child processes during the gRPC + // handshake? c := *http.DefaultClient - tr, err := elasticsearch.NewHTTPTransport(cfg.ESConfig) - if err != nil { - return nil, err - } - c.Transport = apmhttp.WrapRoundTripper(tr) + c.Transport = apmhttp.WrapRoundTripper(http.DefaultTransport) return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.SourceMapConfigs, cfg.Cache.Expiration) } diff --git a/elasticsearch/config.go b/elasticsearch/config.go index 24ef3e6b7bf..91ff11e822e 100644 --- a/elasticsearch/config.go +++ b/elasticsearch/config.go @@ -88,7 +88,7 @@ func connectionConfig(config *Config) (http.RoundTripper, []string, http.Header, if err != nil { return nil, nil, nil, err } - transp, err := NewHTTPTransport(config) + transp, err := httpTransport(config) if err != nil { return nil, nil, nil, err } @@ -134,8 +134,7 @@ func addresses(cfg *Config) ([]string, error) { return addresses, nil } -// NewHttpTransport configures an *http.Transport according to a *Config -func NewHTTPTransport(cfg *Config) (*http.Transport, error) { +func httpTransport(cfg *Config) (*http.Transport, error) { proxy, err := httpProxyURL(cfg) if err != nil { return nil, err From 72a13fed3ff04851f3ecf562dac48dc2ef6f43a7 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 8 Jun 2021 09:23:44 +0200 Subject: [PATCH 19/55] /s/sourcemaps/metadata/ --- beater/beater.go | 4 ++-- beater/config/config_test.go | 8 ++++---- beater/config/rum.go | 28 ++++++++++++++-------------- beater/config/sourcemapping.go | 4 ++-- sourcemap/fleet_store.go | 4 ++-- sourcemap/fleet_store_test.go | 2 +- sourcemap/store_test.go | 6 +++--- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index 1c22a4b92a3..c6630838b5e 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -620,7 +620,7 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { - if len(cfg.SourceMapConfigs) == 0 { + if len(cfg.Metadata) == 0 { c, err := elasticsearch.NewClient(cfg.ESConfig) if err != nil { return nil, err @@ -635,7 +635,7 @@ func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap c := *http.DefaultClient c.Transport = apmhttp.WrapRoundTripper(http.DefaultTransport) - return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.SourceMapConfigs, cfg.Cache.Expiration) + return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.Metadata, cfg.Cache.Expiration) } // WrapRunServerWithProcessors wraps runServer such that it wraps args.Reporter diff --git a/beater/config/config_test.go b/beater/config/config_test.go index f6e7af23bef..ada16f2bd36 100644 --- a/beater/config/config_test.go +++ b/beater/config/config_test.go @@ -194,8 +194,8 @@ func TestUnpackConfig(t *testing.T) { MaxRetries: 3, Backoff: elasticsearch.DefaultBackoffConfig, }, - SourceMapConfigs: []SourceMapConfig{}, - esConfigured: true, + Metadata: []SourceMapMetadata{}, + esConfigured: true, }, LibraryPattern: "^custom", ExcludeFromGrouping: "^grouping", @@ -293,7 +293,7 @@ func TestUnpackConfig(t *testing.T) { "rum": map[string]interface{}{ "enabled": true, "source_mapping": map[string]interface{}{ - "source_maps": []map[string]string{ + "metadata": []map[string]string{ { "service.name": "opbeans-rum", "service.version": "1.2.3", @@ -374,7 +374,7 @@ func TestUnpackConfig(t *testing.T) { }, IndexPattern: "apm-*-sourcemap*", ESConfig: elasticsearch.DefaultConfig(), - SourceMapConfigs: []SourceMapConfig{ + Metadata: []SourceMapMetadata{ { ServiceName: "opbeans-rum", ServiceVersion: "1.2.3", diff --git a/beater/config/rum.go b/beater/config/rum.go index a9c922ba05e..07e2be52433 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -60,12 +60,12 @@ type EventRate struct { // SourceMapping holds sourecemap config information type SourceMapping struct { - Cache Cache `config:"cache"` - Enabled bool `config:"enabled"` - IndexPattern string `config:"index_pattern"` - ESConfig *elasticsearch.Config `config:"elasticsearch"` - SourceMapConfigs []SourceMapConfig `config:"source_maps"` - esConfigured bool + Cache Cache `config:"cache"` + Enabled bool `config:"enabled"` + IndexPattern string `config:"index_pattern"` + ESConfig *elasticsearch.Config `config:"elasticsearch"` + Metadata []SourceMapMetadata `config:"metadata"` + esConfigured bool } func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg *common.Config) error { @@ -80,12 +80,12 @@ func (c *RumConfig) setup(log *logp.Logger, dataStreamsEnabled bool, outputESCfg return errors.Wrapf(err, "Invalid regex for `exclude_from_grouping`: ") } - if c.SourceMapping.esConfigured && len(c.SourceMapping.SourceMapConfigs) > 0 { + if c.SourceMapping.esConfigured && len(c.SourceMapping.Metadata) > 0 { return errors.New("configuring both source_mapping.elasticsearch and sourcemapping.source_maps not allowed") } - // No need to unpack the ESConfig if we've set SourceMapConfigs - if len(c.SourceMapping.SourceMapConfigs) > 0 { + // No need to unpack the ESConfig if SourceMapMetadata exist + if len(c.SourceMapping.Metadata) > 0 { return nil } @@ -112,11 +112,11 @@ func (s *SourceMapping) Unpack(inp *common.Config) error { func defaultSourcemapping() SourceMapping { return SourceMapping{ - Enabled: true, - Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, - IndexPattern: defaultSourcemapIndexPattern, - ESConfig: elasticsearch.DefaultConfig(), - SourceMapConfigs: []SourceMapConfig{}, + Enabled: true, + Cache: Cache{Expiration: defaultSourcemapCacheExpiration}, + IndexPattern: defaultSourcemapIndexPattern, + ESConfig: elasticsearch.DefaultConfig(), + Metadata: []SourceMapMetadata{}, } } diff --git a/beater/config/sourcemapping.go b/beater/config/sourcemapping.go index d14c51116e0..ab98cf44426 100644 --- a/beater/config/sourcemapping.go +++ b/beater/config/sourcemapping.go @@ -17,8 +17,8 @@ package config -// SourceMapConfig holds source map configuration information. -type SourceMapConfig struct { +// SourceMapMetadata holds source map configuration information. +type SourceMapMetadata struct { ServiceName string `config:"service.name"` ServiceVersion string `config:"service.version"` BundleFilepath string `config:"bundle.filepath"` diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index d08b84f24f1..e016abac47a 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -49,7 +49,7 @@ type key struct { func NewFleetStore( c *http.Client, apikey string, - cfgs []config.SourceMapConfig, + cfgs []config.SourceMapMetadata, expiration time.Duration, ) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) @@ -57,7 +57,7 @@ func NewFleetStore( return newStore(s, logger, expiration, 10*time.Second, 25) } -func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapConfig) fleetStore { +func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapMetadata) fleetStore { fleetURLs := make(map[key]string) for _, cfg := range cfgs { k := key{cfg.ServiceName, cfg.ServiceVersion, cfg.BundleFilepath} diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go index ce386c21e63..09ad1e7398f 100644 --- a/sourcemap/fleet_store_test.go +++ b/sourcemap/fleet_store_test.go @@ -46,7 +46,7 @@ func TestFetch(t *testing.T) { })) defer ts.Close() - cfgs := []config.SourceMapConfig{ + cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index fd9ed9ded3d..71e069368c4 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -167,7 +167,7 @@ func TestWaitersLimit(t *testing.T) { })) defer ts.Close() - cfgs := []config.SourceMapConfig{ + cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, @@ -240,7 +240,7 @@ func TestWaitersTimeout(t *testing.T) { })) defer ts.Close() - cfgs := []config.SourceMapConfig{ + cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, @@ -307,7 +307,7 @@ func TestConcurrentFetch(t *testing.T) { })) defer ts.Close() - cfgs := []config.SourceMapConfig{ + cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, From 718735b2335bdbc91296e049671fc345e5b93ae4 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 09:30:06 +0200 Subject: [PATCH 20/55] fix comment --- beater/config/rum.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beater/config/rum.go b/beater/config/rum.go index 07e2be52433..1e61e98b344 100644 --- a/beater/config/rum.go +++ b/beater/config/rum.go @@ -58,7 +58,7 @@ type EventRate struct { LruSize int `config:"lru_size"` } -// SourceMapping holds sourecemap config information +// SourceMapping holds sourcemap config information type SourceMapping struct { Cache Cache `config:"cache"` Enabled bool `config:"enabled"` From dd6cff713e5de85dba7820e284c670376f490381 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 09:35:51 +0200 Subject: [PATCH 21/55] remove sourcemap integration test this can be a unit test --- tests/system/test_integration_sourcemap.py | 23 ---------------------- 1 file changed, 23 deletions(-) diff --git a/tests/system/test_integration_sourcemap.py b/tests/system/test_integration_sourcemap.py index fef3f21bb64..97534fa2a6a 100644 --- a/tests/system/test_integration_sourcemap.py +++ b/tests/system/test_integration_sourcemap.py @@ -208,29 +208,6 @@ def test_rum_transaction(self): assert frames_checked > 0, "no frames checked" -@integration_test -class SourcemapInvalidESConfig(BaseSourcemapTest): - def config(self): - cfg = super(SourcemapInvalidESConfig, self).config() - url = self.split_url(cfg) - cfg.update({ - "smap_es_host": url["host"], - "smap_es_username": url["username"], - "smap_es_password": "xxxx", - }) - return cfg - - def test_unauthorized(self): - # successful - uses output.elasticsearch.* configuration - self.upload_sourcemap() - # unauthorized - uses apm-server.rum.sourcemapping.elasticsearch configuration - self.load_docs_with_template(self.get_error_payload_path(), - self.intake_url, - 'error', - 1) - assert self.log_contains("unable to authenticate user") - - @integration_test class SourcemapESConfigUser(BaseSourcemapTest): def config(self): From be1cd15716ef83bd19ef8eb1277460850b024f06 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 09:38:20 +0200 Subject: [PATCH 22/55] release waiters semaphore if there's a timeout --- sourcemap/store.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sourcemap/store.go b/sourcemap/store.go index d5101a32e50..45819c20de6 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -108,6 +108,8 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str case <-wait: t.Stop() case <-t.C: + // Release back to waiters semaphore + <-s.waiters return nil, errors.New("sourcemap fetch: timeout") } // Release back to waiters semaphore From 041c97a31c1529e079d9cb74c4efd30e014f0987 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:06:09 +0200 Subject: [PATCH 23/55] remove references to version --- beater/config/agentconfig.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beater/config/agentconfig.go b/beater/config/agentconfig.go index 22be77da0c4..30bc9ca4a51 100644 --- a/beater/config/agentconfig.go +++ b/beater/config/agentconfig.go @@ -75,12 +75,12 @@ type Service struct { // String implements the Stringer interface. func (s *Service) String() string { - var name, env, version string + var name, env string if s.Name != "" { name = "service.name=" + s.Name } if s.Environment != "" { env = "service.environment=" + s.Environment } - return strings.Join([]string{name, env, version}, " ") + return strings.Join([]string{name, env}, " ") } From 5bf422ce75865578673348b016adcf4043bdf79f Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:11:15 +0200 Subject: [PATCH 24/55] [store] don't make mutex publicly callable --- sourcemap/store.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index 45819c20de6..926ca8b005a 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -47,7 +47,7 @@ type Store struct { waiters chan struct{} waitTimeout time.Duration - sync.Mutex + mu sync.Mutex inflight map[string]chan struct{} } @@ -90,11 +90,11 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str // if the value hasn't been found, check to see if there's an inflight // request to update the value. - s.Lock() + s.mu.Lock() wait, ok := s.inflight[key] if ok { // found an inflight request, wait for it to complete. - s.Unlock() + s.mu.Unlock() // Check to see if we should wait for a response select { case s.waiters <- struct{}{}: @@ -123,15 +123,15 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str wait = make(chan struct{}) s.inflight[key] = wait - s.Unlock() + s.mu.Unlock() // Once the fetch request is complete, close and remove the channel // from the syncronization map. defer func() { - s.Lock() + s.mu.Lock() delete(s.inflight, key) close(wait) - s.Unlock() + s.mu.Unlock() }() // fetch from Elasticsearch and ensure caching for all non-temporary results From 6948b67bf99d657df61b20c2fa7d84f30b462213 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:13:40 +0200 Subject: [PATCH 25/55] [store] check for ctx.Done() while waiting if at some point the passed in ctx is canceled, we want to respect that also while waiting. --- sourcemap/store.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sourcemap/store.go b/sourcemap/store.go index 926ca8b005a..dc335c51fa9 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -107,6 +107,10 @@ func (s *Store) Fetch(ctx context.Context, name string, version string, path str select { case <-wait: t.Stop() + case <-ctx.Done(): + // Release back to waiters semaphore + <-s.waiters + return nil, ctx.Err() case <-t.C: // Release back to waiters semaphore <-s.waiters From 345b38ba9fcf4479eb37cecb4819800ffae09a05 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:20:00 +0200 Subject: [PATCH 26/55] check for error when reading fleet response --- sourcemap/fleet_store.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index e016abac47a..39d469b9c6e 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -101,7 +101,9 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri buf := new(bytes.Buffer) - io.Copy(buf, resp.Body) + if _, err := io.Copy(buf, resp.Body); err != nil { + return "", err + } return buf.String(), nil } From 7920d64ed59faa84f028594e425201212fd8fe10 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:38:19 +0200 Subject: [PATCH 27/55] [store] verify rum elasticsearch config used --- beater/beater_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/beater/beater_test.go b/beater/beater_test.go index b2123af23f2..55a5a9a02a9 100644 --- a/beater/beater_test.go +++ b/beater/beater_test.go @@ -36,7 +36,9 @@ import ( "go.uber.org/zap/zaptest/observer" "github.com/elastic/apm-server/beater/config" + "github.com/elastic/apm-server/elasticsearch" "github.com/elastic/apm-server/model" + "github.com/elastic/apm-server/sourcemap/test" "github.com/elastic/beats/v7/libbeat/beat" "github.com/elastic/beats/v7/libbeat/common" "github.com/elastic/beats/v7/libbeat/instrumentation" @@ -280,3 +282,27 @@ func TestTransformConfig(t *testing.T) { test(true, false, false) test(true, true, true) } + +func TestStoreUsesRUMElasticsearchConfig(t *testing.T) { + var called bool + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.Write([]byte(test.ValidSourcemap)) + })) + defer ts.Close() + + cfg := config.DefaultConfig() + cfg.RumConfig.Enabled = true + cfg.RumConfig.SourceMapping.Enabled = true + cfg.RumConfig.SourceMapping.ESConfig = elasticsearch.DefaultConfig() + cfg.RumConfig.SourceMapping.ESConfig.Hosts = []string{ts.URL} + + transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg) + require.NoError(t, err) + // Check that the provided rum elasticsearch config was used and + // Fetch() goes to the test server. + _, err = transformConfig.RUM.SourcemapStore.Fetch(context.Background(), "app", "1.0", "/bundle/path") + require.NoError(t, err) + + assert.True(t, called) +} From 6d85b80c91b0dc895559cdc0e8a1e198b0b50422 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 11:38:38 +0200 Subject: [PATCH 28/55] delete repeated types in Fetch() signature --- sourcemap/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index dc335c51fa9..2acd78c2cdd 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -79,7 +79,7 @@ func newStore( } // Fetch a sourcemap from the store. -func (s *Store) Fetch(ctx context.Context, name string, version string, path string) (*sourcemap.Consumer, error) { +func (s *Store) Fetch(ctx context.Context, name, version, path string) (*sourcemap.Consumer, error) { key := cacheKey([]string{name, version, path}) // fetch from cache From 4ae72bb5969299f38252cf67a3f3d0cb5d5d2f64 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 12:01:43 +0200 Subject: [PATCH 29/55] [store] remove waiter limit --- sourcemap/es_store.go | 2 +- sourcemap/fleet_store.go | 2 +- sourcemap/store.go | 24 ++----------- sourcemap/store_test.go | 78 ++-------------------------------------- 4 files changed, 8 insertions(+), 98 deletions(-) diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index 0958e923207..9961e49ff52 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -77,7 +77,7 @@ func NewElasticsearchStore( logger := logp.NewLogger(logs.Sourcemap) s := &esStore{c, index, logger} - return newStore(s, logger, expiration, 10*time.Second, 25) + return newStore(s, logger, expiration, 10*time.Second) } func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 39d469b9c6e..6c6c2c6428b 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -54,7 +54,7 @@ func NewFleetStore( ) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) s := newFleetStore(c, apikey, cfgs) - return newStore(s, logger, expiration, 10*time.Second, 25) + return newStore(s, logger, expiration, 10*time.Second) } func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapMetadata) fleetStore { diff --git a/sourcemap/store.go b/sourcemap/store.go index 2acd78c2cdd..755e66c928c 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -59,20 +59,15 @@ func newStore( b backend, logger *logp.Logger, cacheExpiration, waitTimeout time.Duration, - maxWaiters int, ) (*Store, error) { if cacheExpiration < 0 { return nil, errInit } return &Store{ - cache: gocache.New(cacheExpiration, cleanupInterval(cacheExpiration)), - backend: b, - logger: logger, - // TODO: Number of requests waiting for a sourcemap to be - // fetched. Any suggestion as to where we should set this is - // appreciated. - waiters: make(chan struct{}, maxWaiters), + cache: gocache.New(cacheExpiration, cleanupInterval(cacheExpiration)), + backend: b, + logger: logger, waitTimeout: waitTimeout, inflight: make(map[string]chan struct{}), }, nil @@ -95,29 +90,16 @@ func (s *Store) Fetch(ctx context.Context, name, version, path string) (*sourcem if ok { // found an inflight request, wait for it to complete. s.mu.Unlock() - // Check to see if we should wait for a response - select { - case s.waiters <- struct{}{}: - default: - // waiters is currently full, abort the request. - return nil, errors.New("sourcemap fetch: too many open requests") - } t := time.NewTimer(s.waitTimeout) select { case <-wait: t.Stop() case <-ctx.Done(): - // Release back to waiters semaphore - <-s.waiters return nil, ctx.Err() case <-t.C: - // Release back to waiters semaphore - <-s.waiters return nil, errors.New("sourcemap fetch: timeout") } - // Release back to waiters semaphore - <-s.waiters // Try to read the value again return s.Fetch(ctx, name, version, path) } diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 71e069368c4..76d27a7a619 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -43,10 +43,10 @@ import ( func Test_newStore(t *testing.T) { logger := logp.NewLogger(logs.Sourcemap) - _, err := newStore(nil, logger, -1, time.Second, 25) + _, err := newStore(nil, logger, -1, time.Second) require.Error(t, err) - f, err := newStore(nil, logger, 100, time.Second, 25) + f, err := newStore(nil, logger, 100, time.Second) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -151,78 +151,6 @@ func TestStore_Fetch(t *testing.T) { }) } -func TestWaitersLimit(t *testing.T) { - var ( - errs int64 - - apikey = "supersecret" - name = "webapp" - version = "1.0.0" - path = "/my/path/to/bundle.js.map" - c = http.DefaultClient - ) - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(50 * time.Millisecond) - w.Write([]byte(test.ValidSourcemap)) - })) - defer ts.Close() - - cfgs := []config.SourceMapMetadata{ - { - ServiceName: name, - ServiceVersion: version, - BundleFilepath: path, - SourceMapURL: ts.URL, - }, - { - ServiceName: name, - ServiceVersion: "1.1.0", - BundleFilepath: path, - SourceMapURL: ts.URL, - }, - } - store, err := NewFleetStore(c, apikey, cfgs, time.Minute) - assert.NoError(t, err) - - var wg sync.WaitGroup - // NewFleetStore is currently limited to 1 active + 25 waiting = 26 - // requests, so the 27th should fail. All others wait and receive a - // successful response. - for i := 0; i < 27; i++ { - wg.Add(1) - go func() { - consumer, err := store.Fetch(context.Background(), name, version, path) - if err != nil { - assert.Nil(t, consumer) - atomic.AddInt64(&errs, 1) - } else { - assert.NotNil(t, consumer) - } - - wg.Done() - }() - } - - wg.Wait() - - assert.Equal(t, int64(1), errs) - - // Verify that the semaphore was released and requests for a different - // key work as expected. - for i := 0; i < 26; i++ { - wg.Add(1) - go func() { - consumer, err := store.Fetch(context.Background(), name, "1.1.0", path) - assert.NoError(t, err) - assert.NotNil(t, consumer) - - wg.Done() - }() - } - - wg.Wait() -} - func TestWaitersTimeout(t *testing.T) { var ( errs int64 @@ -250,7 +178,7 @@ func TestWaitersTimeout(t *testing.T) { } b := newFleetStore(c, apikey, cfgs) logger := logp.NewLogger(logs.Sourcemap) - store, err := newStore(b, logger, time.Minute, time.Millisecond, 25) + store, err := newStore(b, logger, time.Minute, time.Millisecond) assert.NoError(t, err) var wg sync.WaitGroup From 3650b4841ad87d6239ebffe2646facbecfa5080b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 12:31:21 +0200 Subject: [PATCH 30/55] remove waiters struct member --- sourcemap/store.go | 1 - 1 file changed, 1 deletion(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index 755e66c928c..d059455fabf 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -44,7 +44,6 @@ type Store struct { cache *gocache.Cache backend backend logger *logp.Logger - waiters chan struct{} waitTimeout time.Duration mu sync.Mutex From 7d2e73da699f304b462ada547f58a08c1ae8227c Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 9 Jun 2021 12:31:35 +0200 Subject: [PATCH 31/55] wrap comment to 80 char --- sourcemap/store.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sourcemap/store.go b/sourcemap/store.go index d059455fabf..e6fe29ce019 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -39,7 +39,8 @@ var ( errInit = errors.New("Cache cannot be initialized. Expiration and CleanupInterval need to be >= 0") ) -// Store holds information necessary to fetch a sourcemap, either from an Elasticsearch instance or an internal cache. +// Store holds information necessary to fetch a sourcemap, either from an +// Elasticsearch instance or an internal cache. type Store struct { cache *gocache.Cache backend backend From 2bbf00b1073af621a7a63d0d5c7ad0c5b7b36728 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 10 Jun 2021 21:11:43 +0800 Subject: [PATCH 32/55] systemtest: add rate limiting test (#5430) Check that rate limiting is wired up, rely on unit tests to cover more specific scenarios. --- systemtest/apmservertest/config.go | 9 ++++ systemtest/rum_test.go | 48 ++++++++++++++++++++ tests/system/test_requests.py | 71 ------------------------------ 3 files changed, 57 insertions(+), 71 deletions(-) diff --git a/systemtest/apmservertest/config.go b/systemtest/apmservertest/config.go index 388106237c3..cec61590c9f 100644 --- a/systemtest/apmservertest/config.go +++ b/systemtest/apmservertest/config.go @@ -156,6 +156,15 @@ type RUMConfig struct { // ResponseHeaders holds headers to add to all APM Server RUM HTTP responses. ResponseHeaders http.Header `json:"response_headers,omitempty"` + + // RateLimit holds event rate limit configuration. + RateLimit *RUMRateLimitConfig `json:"event_rate,omitempty"` +} + +// RUMRateLimitConfig holds APM Server RUM event rate limit configuration. +type RUMRateLimitConfig struct { + IPLimit int `json:"lru_size,omitempty"` + EventLimit int `json:"limit,omitempty"` } // DataStreamsConfig holds APM Server data streams configuration. diff --git a/systemtest/rum_test.go b/systemtest/rum_test.go index 5da1d07f396..93c7d82f84d 100644 --- a/systemtest/rum_test.go +++ b/systemtest/rum_test.go @@ -19,6 +19,7 @@ package systemtest_test import ( "bytes" + "fmt" "io" "io/ioutil" "math/rand" @@ -34,6 +35,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" "github.com/elastic/apm-server/systemtest" "github.com/elastic/apm-server/systemtest/apmservertest" @@ -146,6 +148,52 @@ func TestRUMAllowServiceNames(t *testing.T) { assert.Equal(t, `{"accepted":0,"errors":[{"message":"service name is not allowed"}]}`+"\n", string(respBody)) } +func TestRUMRateLimit(t *testing.T) { + srv := apmservertest.NewUnstartedServer(t) + srv.Config.RUM = &apmservertest.RUMConfig{ + Enabled: true, + RateLimit: &apmservertest.RUMRateLimitConfig{ + IPLimit: 2, + EventLimit: 10, + }, + } + err := srv.Start() + require.NoError(t, err) + + sendEvents := func(ip string, n int) error { + body := bytes.NewBufferString(`{"metadata":{"service":{"name":"allowed","version":"1.0.0","agent":{"name":"rum-js","version":"0.0.0"}}}}` + "\n") + for i := 0; i < n; i++ { + body.WriteString(`{"transaction":{"trace_id":"x","id":"y","type":"z","duration":0,"span_count":{"started":1},"context":{"service":{"name":"foo"}}}}` + "\n") + } + + req, _ := http.NewRequest("POST", srv.URL+"/intake/v2/rum/events?verbose=true", body) + req.Header.Add("Content-Type", "application/x-ndjson") + req.Header.Add("X-Forwarded-For", ip) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + respBody, _ := ioutil.ReadAll(resp.Body) + if resp.StatusCode != http.StatusAccepted { + return fmt.Errorf("%s (%s)", resp.Status, strings.TrimSpace(string(respBody))) + } + return nil + } + + // Just check that rate limiting is wired up. More specific rate limiting scenarios are unit tested. + + var g errgroup.Group + g.Go(func() error { return sendEvents("10.11.12.13", srv.Config.RUM.RateLimit.EventLimit) }) + g.Go(func() error { return sendEvents("10.11.12.14", srv.Config.RUM.RateLimit.EventLimit) }) + assert.NoError(t, g.Wait()) + + g = errgroup.Group{} + g.Go(func() error { return sendEvents("10.11.12.13", srv.Config.RUM.RateLimit.EventLimit) }) + g.Go(func() error { return sendEvents("10.11.12.14", srv.Config.RUM.RateLimit.EventLimit) }) + g.Go(func() error { return sendEvents("10.11.12.15", srv.Config.RUM.RateLimit.EventLimit) }) + assert.EqualError(t, g.Wait(), `429 Too Many Requests ({"accepted":0,"errors":[{"message":"rate limit exceeded"}]})`) +} + func sendRUMEventsPayload(t *testing.T, srv *apmservertest.Server, payloadFile string) { t.Helper() diff --git a/tests/system/test_requests.py b/tests/system/test_requests.py index 515ff4c6777..c88fb012219 100644 --- a/tests/system/test_requests.py +++ b/tests/system/test_requests.py @@ -145,74 +145,3 @@ def test_preflight_bad_headers(self): assert 'Access-Control-Allow-Origin' not in r.headers.keys(), r.headers assert r.headers['Access-Control-Allow-Headers'] == 'Content-Type, Content-Encoding, Accept', r.headers assert r.headers['Access-Control-Allow-Methods'] == 'POST, OPTIONS', r.headers - - -class RateLimitTest(ClientSideBaseTest): - - def fire_events(self, data_file, iterations, split_ips=False): - events = self.get_event_payload(name=data_file) - threads = [] - codes = defaultdict(int) - - def fire(x): - ip = '10.11.12.13' - if split_ips and x % 2: - ip = '10.11.12.14' - r = self.request_intake(data=events, - headers={'content-type': 'application/x-ndjson', 'X-Forwarded-For': ip}) - codes[r.status_code] += 1 - return r.status_code - - # rate limit hit, because every event in request is counted - for x in range(iterations): - threads.append(threading.Thread(target=fire, args=(x,))) - - for t in threads: - t.start() - time.sleep(0.01) - - for t in threads: - t.join() - return codes - - # limit: 16, burst_multiplier: 3, burst: 48 - def test_rate_limit(self): - # all requests from the same ip - # 19 events, batch size 10 => 20+1 events per requ - codes = self.fire_events("ratelimit.ndjson", 3) - assert set(codes.keys()) == set([202]), codes - - def test_rate_limit_hit(self): - # all requests from the same ip - codes = self.fire_events("ratelimit.ndjson", 5) - assert set(codes.keys()) == set([202, 429]), codes - assert codes[429] == 2, codes - assert codes[202] == 3, codes - - def test_rate_limit_small_hit(self): - # all requests from the same ip - # 4 events, batch size 10 => 10+1 events per requ - codes = self.fire_events("events.ndjson", 8) - assert set(codes.keys()) == set([202, 429]), codes - assert codes[429] == 3, codes - assert codes[202] == 5, codes - - def test_rate_limit_only_metadata(self): - # all requests from the same ip - # no events, batch size 10 => 10+1 events per requ - codes = self.fire_events("metadata.ndjson", 8) - assert set(codes.keys()) == set([202, 429]), codes - assert codes[429] == 3, codes - assert codes[202] == 5, codes - - def test_multiple_ips_rate_limit(self): - # requests from 2 different ips - codes = self.fire_events("ratelimit.ndjson", 6, True) - assert set(codes.keys()) == set([202]), codes - - def test_multiple_ips_rate_limit_hit(self): - # requests from 2 different ips - codes = self.fire_events("ratelimit.ndjson", 10, True) - assert set(codes.keys()) == set([202, 429]), codes - assert codes[429] == 4, codes - assert codes[202] == 6, codes From 93f7b4473f391e9c94843d1684651dac692bfdfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20=C3=81lvarez?= Date: Thu, 10 Jun 2021 21:57:03 +0200 Subject: [PATCH 33/55] Hardcode troughput settings for cloud environment (#5402) --- cmd/root.go | 71 ++++++++++++++++++++++++++++++++++++++++-------- cmd/root_test.go | 53 ++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 12 deletions(-) create mode 100644 cmd/root_test.go diff --git a/cmd/root.go b/cmd/root.go index 2dfcd252ddb..faa21d17000 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -19,6 +19,7 @@ package cmd import ( "fmt" + "os" "github.com/spf13/pflag" @@ -37,22 +38,68 @@ import ( const ( beatName = "apm-server" apmIndexPattern = "apm" + cloudEnv = "CLOUD_APM_CAPACITY" ) -var libbeatConfigOverrides = []cfgfile.ConditionalOverride{{ - Check: func(_ *common.Config) bool { - return true +type throughputSettings struct { + worker int + bulkMaxSize int + events int + minEvents int +} + +var cloudMatrix = map[string]throughputSettings{ + "512": {5, 267, 2000, 267}, + "1024": {7, 381, 4000, 381}, + "2048": {10, 533, 8000, 533}, + "4096": {14, 762, 16000, 762}, + "8192": {20, 1067, 32000, 1067}, +} + +var libbeatConfigOverrides = func() []cfgfile.ConditionalOverride { + return []cfgfile.ConditionalOverride{{ + Check: func(_ *common.Config) bool { + return true + }, + Config: common.MustNewConfigFrom(map[string]interface{}{ + "logging": map[string]interface{}{ + "metrics": map[string]interface{}{ + "enabled": false, + }, + "ecs": true, + "json": true, + }, + }), }, - Config: common.MustNewConfigFrom(map[string]interface{}{ - "logging": map[string]interface{}{ - "metrics": map[string]interface{}{ - "enabled": false, + { + Check: func(_ *common.Config) bool { + return true }, - "ecs": true, - "json": true, + Config: func() *common.Config { + cap := os.Getenv(cloudEnv) + if _, ok := cloudMatrix[cap]; !ok { + return common.NewConfig() + } + return common.MustNewConfigFrom(map[string]interface{}{ + "output": map[string]interface{}{ + "elasticsearch": map[string]interface{}{ + "worker": cloudMatrix[cap].worker, + "bulk_max_size": cloudMatrix[cap].bulkMaxSize, + }, + }, + "queue": map[string]interface{}{ + "mem": map[string]interface{}{ + "events": cloudMatrix[cap].events, + "flush": map[string]interface{}{ + "min_events": cloudMatrix[cap].minEvents, + }, + }, + }, + }) + }(), }, - }), -}} + } +} // DefaultSettings return the default settings for APM Server to pass into // the GenRootCmdWithSettings. @@ -67,7 +114,7 @@ func DefaultSettings() instance.Settings { }, IndexManagement: idxmgmt.MakeDefaultSupporter, Processing: processing.MakeDefaultObserverSupport(false), - ConfigOverrides: libbeatConfigOverrides, + ConfigOverrides: libbeatConfigOverrides(), } } diff --git a/cmd/root_test.go b/cmd/root_test.go new file mode 100644 index 00000000000..daa7025ea9d --- /dev/null +++ b/cmd/root_test.go @@ -0,0 +1,53 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package cmd + +import ( + "os" + "testing" + + "github.com/elastic/beats/v7/libbeat/common" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCloudEnv(t *testing.T) { + defer os.Unsetenv(cloudEnv) + + // no cloud environment variable set + settings := DefaultSettings() + assert.Len(t, settings.ConfigOverrides, 2) + assert.Equal(t, common.NewConfig(), settings.ConfigOverrides[1].Config) + + // cloud environment picked up + os.Setenv(cloudEnv, "512") + settings = DefaultSettings() + assert.Len(t, settings.ConfigOverrides, 2) + cfg := settings.ConfigOverrides[1].Config + assert.NotNil(t, cfg) + workers, err := cfg.Int("output.elasticsearch.worker", -1) + require.NoError(t, err) + assert.Equal(t, int64(5), workers) + + // bad cloud environment value + os.Setenv(cloudEnv, "123") + settings = DefaultSettings() + assert.Len(t, settings.ConfigOverrides, 2) + assert.Equal(t, common.NewConfig(), settings.ConfigOverrides[1].Config) +} From 6859ecba1673079def896d8d1a2735d79b318c18 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Mon, 14 Jun 2021 15:26:07 +0800 Subject: [PATCH 34/55] systemtest: fix apm-server binary injection (#5440) Due to some changes in elastic-agent (https://github.com/elastic/beats/pull/24817), injection of the apm-server binary became ineffective and we have been running system tests with the published artifacts. Artifacts (such as the apm-server) are now unpacked into state/data/install/. The state/data/install directory is expected to be owned by the elastic-agent user, so we can no longer bind mount the apm-server binary. Instead, we now create a custom Docker image and copy in the apm-server and apm-server.yml files. --- systemtest/containers.go | 155 ++++++++++++++++++++++++++++----------- systemtest/fleet_test.go | 23 ------ 2 files changed, 111 insertions(+), 67 deletions(-) diff --git a/systemtest/containers.go b/systemtest/containers.go index 161444c4a3a..d50f0d720b6 100644 --- a/systemtest/containers.go +++ b/systemtest/containers.go @@ -18,6 +18,8 @@ package systemtest import ( + "archive/tar" + "bytes" "context" "encoding/json" "errors" @@ -29,8 +31,8 @@ import ( "net/url" "os" "os/exec" - "path" "path/filepath" + "runtime" "strings" "time" @@ -42,6 +44,7 @@ import ( "github.com/testcontainers/testcontainers-go/wait" "golang.org/x/sync/errgroup" + "github.com/elastic/apm-server/systemtest/apmservertest" "github.com/elastic/apm-server/systemtest/estest" "github.com/elastic/go-elasticsearch/v7" ) @@ -281,6 +284,11 @@ func NewUnstartedElasticAgentContainer() (*ElasticAgentContainer, error) { Scheme: "https", Host: net.JoinHostPort(fleetServerIPAddress, fleetServerPort), } + containerCACertPath := "/etc/pki/tls/certs/fleet-ca.pem" + hostCACertPath, err := filepath.Abs("../testing/docker/fleet-server/ca.pem") + if err != nil { + return nil, err + } // Use the same stack version as used for fleet-server. agentImageVersion := fleetServerContainer.Image[strings.LastIndex(fleetServerContainer.Image, ":")+1:] @@ -292,43 +300,34 @@ func NewUnstartedElasticAgentContainer() (*ElasticAgentContainer, error) { if err != nil { return nil, err } - agentVCSRef := agentImageDetails.Config.Labels["org.label-schema.vcs-ref"] - agentDataHashDir := path.Join("/usr/share/elastic-agent/data", "elastic-agent-"+agentVCSRef[:6]) - agentInstallDir := path.Join(agentDataHashDir, "install") + stackVersion := agentImageDetails.Config.Labels["org.label-schema.version"] + + // Build a custom elastic-agent image with a locally built apm-server binary injected. + agentImage, err = buildElasticAgentImage(context.Background(), docker, stackVersion, agentImageVersion) + if err != nil { + return nil, err + } req := testcontainers.ContainerRequest{ Image: agentImage, AutoRemove: true, Networks: networks, + BindMounts: map[string]string{hostCACertPath: containerCACertPath}, Env: map[string]string{ - // NOTE(axw) because we bind-mount the apm-server artifacts in, they end up owned by the - // current user rather than root. Disable Beats's strict permission checks to avoid resulting - // complaints, as they're irrelevant to these system tests. - "BEAT_STRICT_PERMS": "false", + "FLEET_URL": fleetServerURL.String(), + "FLEET_CA": containerCACertPath, }, } return &ElasticAgentContainer{ - request: req, - installDir: agentInstallDir, - fleetServerURL: fleetServerURL.String(), - StackVersion: agentImageVersion, - BindMountInstall: make(map[string]string), + request: req, + StackVersion: agentImageVersion, }, nil } // ElasticAgentContainer represents an ephemeral Elastic Agent container. type ElasticAgentContainer struct { - container testcontainers.Container - request testcontainers.ContainerRequest - fleetServerURL string - - // installDir holds the location of the "install" directory inside - // the Elastic Agent container. - // - // This will be set when the ElasticAgentContainer object is created, - // and can be used to anticipate the location into which artifacts - // can be bind-mounted. - installDir string + container testcontainers.Container + request testcontainers.ContainerRequest // StackVersion holds the stack version of the container image, // e.g. 8.0.0-SNAPSHOT. @@ -344,11 +343,6 @@ type ElasticAgentContainer struct { // by exposed port. This will be populated by Start. Addrs map[string]string - // BindMountInstall holds a map of files to bind mount into the - // container, mapping from the host location to target paths relative - // to the install directory in the container. - BindMountInstall map[string]string - // FleetEnrollmentToken holds an optional Fleet enrollment token to // use for enrolling the agent with Fleet. The agent will only enroll // if this is specified. @@ -366,27 +360,12 @@ func (c *ElasticAgentContainer) Start() error { defer cancel() // Update request from user-definable fields. - c.request.Env["FLEET_URL"] = c.fleetServerURL if c.FleetEnrollmentToken != "" { c.request.Env["FLEET_ENROLL"] = "1" c.request.Env["FLEET_ENROLLMENT_TOKEN"] = c.FleetEnrollmentToken } - c.request.ExposedPorts = c.ExposedPorts c.request.WaitingFor = c.WaitingFor - c.request.BindMounts = map[string]string{} - for source, target := range c.BindMountInstall { - c.request.BindMounts[source] = path.Join(c.installDir, target) - } - - // Inject CA certificate for verifying fleet-server. - containerCACertPath := "/etc/pki/tls/certs/fleet-ca.pem" - hostCACertPath, err := filepath.Abs("../testing/docker/fleet-server/ca.pem") - if err != nil { - return err - } - c.request.BindMounts[hostCACertPath] = containerCACertPath - c.request.Env["FLEET_CA"] = containerCACertPath container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ ContainerRequest: c.request, @@ -457,3 +436,91 @@ func matchFleetServerAPIStatusHealthy(r io.Reader) bool { } return status.Status == "HEALTHY" } + +// buildElasticAgentImage builds a Docker image from the published image with a locally built apm-server injected. +func buildElasticAgentImage(ctx context.Context, docker *client.Client, stackVersion, imageVersion string) (string, error) { + imageName := fmt.Sprintf("elastic-agent-systemtest:%s", imageVersion) + log.Printf("Building image %s...", imageName) + + // Build apm-server, and copy it into the elastic-agent container's "install" directory. + // This bypasses downloading the artifact. + arch := runtime.GOARCH + if arch == "amd64" { + arch = "x86_64" + } + apmServerInstallDir := fmt.Sprintf("./state/data/install/apm-server-%s-linux-%s", stackVersion, arch) + apmServerBinary, err := apmservertest.BuildServerBinary("linux") + if err != nil { + return "", err + } + + // Binaries to copy from disk into the build context. + binaries := map[string]string{ + "apm-server": apmServerBinary, + } + + // Generate Dockerfile contents. + var dockerfile bytes.Buffer + fmt.Fprintf(&dockerfile, "FROM docker.elastic.co/beats/elastic-agent:%s\n", imageVersion) + fmt.Fprintf(&dockerfile, "COPY --chown=elastic-agent:elastic-agent apm-server apm-server.yml %s/\n", apmServerInstallDir) + + // Files to generate in the build context. + generatedFiles := map[string][]byte{ + "Dockerfile": dockerfile.Bytes(), + "apm-server.yml": []byte(""), + } + + var buildContext bytes.Buffer + tarw := tar.NewWriter(&buildContext) + for name, path := range binaries { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + info, err := f.Stat() + if err != nil { + return "", err + } + if err := tarw.WriteHeader(&tar.Header{ + Name: name, + Size: info.Size(), + Mode: 0755, + Uname: "elastic-agent", + Gname: "elastic-agent", + }); err != nil { + return "", err + } + if _, err := io.Copy(tarw, f); err != nil { + return "", err + } + } + for name, content := range generatedFiles { + if err := tarw.WriteHeader(&tar.Header{ + Name: name, + Size: int64(len(content)), + Mode: 0644, + Uname: "elastic-agent", + Gname: "elastic-agent", + }); err != nil { + return "", err + } + if _, err := tarw.Write(content); err != nil { + return "", err + } + } + if err := tarw.Close(); err != nil { + return "", err + } + + resp, err := docker.ImageBuild(ctx, &buildContext, types.ImageBuildOptions{Tags: []string{imageName}}) + if err != nil { + return "", err + } + defer resp.Body.Close() + if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil { + return "", err + } + log.Printf("Built image %s", imageName) + return imageName, nil +} diff --git a/systemtest/fleet_test.go b/systemtest/fleet_test.go index aad92b43574..761bccaad3c 100644 --- a/systemtest/fleet_test.go +++ b/systemtest/fleet_test.go @@ -19,12 +19,8 @@ package systemtest_test import ( "context" - "fmt" "io/ioutil" "net/url" - "path" - "path/filepath" - "runtime" "testing" "time" @@ -35,7 +31,6 @@ import ( "go.elastic.co/apm/transport" "github.com/elastic/apm-server/systemtest" - "github.com/elastic/apm-server/systemtest/apmservertest" "github.com/elastic/apm-server/systemtest/fleettest" ) @@ -80,24 +75,6 @@ func TestFleetIntegration(t *testing.T) { } }() - // Build apm-server, and bind-mount it into the elastic-agent container's "install" - // directory. This bypasses downloading the artifact. - arch := runtime.GOARCH - if arch == "amd64" { - arch = "x86_64" - } - apmServerArtifactName := fmt.Sprintf("apm-server-%s-linux-%s", agent.StackVersion, arch) - - // Bind-mount the apm-server binary and apm-server.yml into the container's - // "install" directory. This causes elastic-agent to skip installing the - // artifact. - apmServerBinary, err := apmservertest.BuildServerBinary("linux") - require.NoError(t, err) - agent.BindMountInstall[apmServerBinary] = path.Join(apmServerArtifactName, "apm-server") - apmServerConfigFile, err := filepath.Abs("../apm-server.yml") - require.NoError(t, err) - agent.BindMountInstall[apmServerConfigFile] = path.Join(apmServerArtifactName, "apm-server.yml") - // Start elastic-agent with port 8200 exposed, and wait for the server to service // healthcheck requests to port 8200. agent.ExposedPorts = []string{"8200"} From 71a8792b23e3b4239d714143b6a577b6c8fbe383 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 14 Jun 2021 11:16:35 +0200 Subject: [PATCH 35/55] remove timeout on waiting for fetch leave this as the responsibility of the caller, to decide how long they want to wait. --- sourcemap/es_store.go | 2 +- sourcemap/fleet_store.go | 2 +- sourcemap/store.go | 22 ++++++++-------------- sourcemap/store_test.go | 17 +++++++++++------ 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/sourcemap/es_store.go b/sourcemap/es_store.go index 9961e49ff52..ad999f4d17e 100644 --- a/sourcemap/es_store.go +++ b/sourcemap/es_store.go @@ -77,7 +77,7 @@ func NewElasticsearchStore( logger := logp.NewLogger(logs.Sourcemap) s := &esStore{c, index, logger} - return newStore(s, logger, expiration, 10*time.Second) + return newStore(s, logger, expiration) } func (s *esStore) fetch(ctx context.Context, name, version, path string) (string, error) { diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 6c6c2c6428b..7c7003f31b0 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -54,7 +54,7 @@ func NewFleetStore( ) (*Store, error) { logger := logp.NewLogger(logs.Sourcemap) s := newFleetStore(c, apikey, cfgs) - return newStore(s, logger, expiration, 10*time.Second) + return newStore(s, logger, expiration) } func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapMetadata) fleetStore { diff --git a/sourcemap/store.go b/sourcemap/store.go index e6fe29ce019..aec47b54833 100644 --- a/sourcemap/store.go +++ b/sourcemap/store.go @@ -42,10 +42,9 @@ var ( // Store holds information necessary to fetch a sourcemap, either from an // Elasticsearch instance or an internal cache. type Store struct { - cache *gocache.Cache - backend backend - logger *logp.Logger - waitTimeout time.Duration + cache *gocache.Cache + backend backend + logger *logp.Logger mu sync.Mutex inflight map[string]chan struct{} @@ -58,18 +57,17 @@ type backend interface { func newStore( b backend, logger *logp.Logger, - cacheExpiration, waitTimeout time.Duration, + cacheExpiration time.Duration, ) (*Store, error) { if cacheExpiration < 0 { return nil, errInit } return &Store{ - cache: gocache.New(cacheExpiration, cleanupInterval(cacheExpiration)), - backend: b, - logger: logger, - waitTimeout: waitTimeout, - inflight: make(map[string]chan struct{}), + cache: gocache.New(cacheExpiration, cleanupInterval(cacheExpiration)), + backend: b, + logger: logger, + inflight: make(map[string]chan struct{}), }, nil } @@ -91,14 +89,10 @@ func (s *Store) Fetch(ctx context.Context, name, version, path string) (*sourcem // found an inflight request, wait for it to complete. s.mu.Unlock() - t := time.NewTimer(s.waitTimeout) select { case <-wait: - t.Stop() case <-ctx.Done(): return nil, ctx.Err() - case <-t.C: - return nil, errors.New("sourcemap fetch: timeout") } // Try to read the value again return s.Fetch(ctx, name, version, path) diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 76d27a7a619..b2a9ad274a7 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -19,6 +19,7 @@ package sourcemap import ( "context" + "errors" "fmt" "net/http" "net/http/httptest" @@ -43,10 +44,10 @@ import ( func Test_newStore(t *testing.T) { logger := logp.NewLogger(logs.Sourcemap) - _, err := newStore(nil, logger, -1, time.Second) + _, err := newStore(nil, logger, -1) require.Error(t, err) - f, err := newStore(nil, logger, 100, time.Second) + f, err := newStore(nil, logger, 100) require.NoError(t, err) assert.NotNil(t, f.cache) } @@ -151,7 +152,7 @@ func TestStore_Fetch(t *testing.T) { }) } -func TestWaitersTimeout(t *testing.T) { +func TestFetchTimeout(t *testing.T) { var ( errs int64 @@ -178,16 +179,20 @@ func TestWaitersTimeout(t *testing.T) { } b := newFleetStore(c, apikey, cfgs) logger := logp.NewLogger(logs.Sourcemap) - store, err := newStore(b, logger, time.Minute, time.Millisecond) + store, err := newStore(b, logger, time.Minute) assert.NoError(t, err) + ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) + defer cancel() + var wg sync.WaitGroup for i := 0; i < 2; i++ { wg.Add(1) go func() { - consumer, err := store.Fetch(context.Background(), name, version, path) + consumer, err := store.Fetch(ctx, name, version, path) if err != nil { - assert.Contains(t, err.Error(), "timeout") + + assert.True(t, errors.Is(err, context.DeadlineExceeded)) atomic.AddInt64(&errs, 1) close(waitc) } else { From 1a9676ec20231908f68ee52a6f36e0105ac1a456 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 14 Jun 2021 11:23:05 +0200 Subject: [PATCH 36/55] use provided fleetmode.Enabled() use the provided lib to decide if we're in fleetmode. if true, then always use direct agent config. --- beater/beater.go | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index 89207e56dbb..d2bb64c8964 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -625,22 +625,21 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { - if len(cfg.Metadata) == 0 { - c, err := elasticsearch.NewClient(cfg.ESConfig) - if err != nil { - return nil, err - } - index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) - return sourcemap.NewElasticsearchStore(c, index, cfg.Cache.Expiration) - } - - // TODO: Configure the client with fleet TLS certs, timeouts, etc. - // Fleet hands TLS certs to the child processes during the gRPC - // handshake? - c := *http.DefaultClient + if fleetmode.Enabled() { + // TODO: Configure the client with fleet TLS certs, timeouts, etc. + // Fleet hands TLS certs to the child processes during the gRPC + // handshake? + c := *http.DefaultClient - c.Transport = apmhttp.WrapRoundTripper(http.DefaultTransport) - return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.Metadata, cfg.Cache.Expiration) + c.Transport = apmhttp.WrapRoundTripper(http.DefaultTransport) + return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.Metadata, cfg.Cache.Expiration) + } + c, err := elasticsearch.NewClient(cfg.ESConfig) + if err != nil { + return nil, err + } + index := strings.ReplaceAll(cfg.IndexPattern, "%{[observer.version]}", beatInfo.Version) + return sourcemap.NewElasticsearchStore(c, index, cfg.Cache.Expiration) } // WrapRunServerWithProcessors wraps runServer such that it wraps args.Reporter From 47223ccafd1652de2ae93b320176f699fa4a712e Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 14 Jun 2021 12:20:56 +0200 Subject: [PATCH 37/55] [fleet] add tls fields to config incoming config provides us with TLS config and connection info, but we have been ignoring it. --- beater/config/integration.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/beater/config/integration.go b/beater/config/integration.go index 99c5c321e15..e3c9dc246f1 100644 --- a/beater/config/integration.go +++ b/beater/config/integration.go @@ -21,6 +21,7 @@ import ( "errors" "github.com/elastic/beats/v7/libbeat/common" + "github.com/elastic/beats/v7/libbeat/common/transport/tlscommon" "github.com/elastic/beats/v7/libbeat/kibana" ) @@ -66,5 +67,9 @@ type Package struct { } type Fleet struct { - Kibana kibana.ClientConfig `config:"kibana"` + Kibana kibana.ClientConfig `config:"kibana"` + Hosts []string `config:"hosts"` + Protocol string `config:"protocol"` + AccessAPIKey string `config:"access_api_key"` + TLS *tlscommon.Config `config:"ssl"` } From fee6582171dcce4913a2904281675c857168bd1b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Mon, 14 Jun 2021 12:24:58 +0200 Subject: [PATCH 38/55] [fleet] remove kibana config references no longer needed kibana configuration --- beater/beater.go | 14 +++----------- beater/config/integration.go | 10 ++++------ 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index d2bb64c8964..28868363c13 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -29,8 +29,6 @@ import ( "github.com/elastic/beats/v7/libbeat/common/fleetmode" - "github.com/elastic/beats/v7/libbeat/kibana" - "github.com/pkg/errors" "go.elastic.co/apm" "go.elastic.co/apm/module/apmhttp" @@ -258,7 +256,6 @@ func (s *serverCreator) Create(p beat.PipelineConnector, rawConfig *common.Confi sharedServerRunnerParams: s.args, Namespace: namespace, Pipeline: p, - KibanaConfig: &integrationConfig.Fleet.Kibana, RawConfig: apmServerCommonConfig, }) } @@ -291,10 +288,9 @@ type serverRunner struct { type serverRunnerParams struct { sharedServerRunnerParams - Namespace string - Pipeline beat.PipelineConnector - KibanaConfig *kibana.ClientConfig - RawConfig *common.Config + Namespace string + Pipeline beat.PipelineConnector + RawConfig *common.Config } type sharedServerRunnerParams struct { @@ -312,10 +308,6 @@ func newServerRunner(ctx context.Context, args serverRunnerParams) (*serverRunne return nil, err } - if cfg.DataStreams.Enabled && args.KibanaConfig != nil { - cfg.Kibana.ClientConfig = *args.KibanaConfig - } - runServerContext, cancel := context.WithCancel(ctx) return &serverRunner{ backgroundContext: ctx, diff --git a/beater/config/integration.go b/beater/config/integration.go index e3c9dc246f1..9c6c61d509f 100644 --- a/beater/config/integration.go +++ b/beater/config/integration.go @@ -22,7 +22,6 @@ import ( "github.com/elastic/beats/v7/libbeat/common" "github.com/elastic/beats/v7/libbeat/common/transport/tlscommon" - "github.com/elastic/beats/v7/libbeat/kibana" ) func NewIntegrationConfig(rootConfig *common.Config) (*IntegrationConfig, error) { @@ -67,9 +66,8 @@ type Package struct { } type Fleet struct { - Kibana kibana.ClientConfig `config:"kibana"` - Hosts []string `config:"hosts"` - Protocol string `config:"protocol"` - AccessAPIKey string `config:"access_api_key"` - TLS *tlscommon.Config `config:"ssl"` + Hosts []string `config:"hosts"` + Protocol string `config:"protocol"` + AccessAPIKey string `config:"access_api_key"` + TLS *tlscommon.Config `config:"ssl"` } From 90a7318da4d28596b06b2177b3a5872d197d0427 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 16 Jun 2021 14:38:45 +0200 Subject: [PATCH 39/55] add conditional around rendering api-key only render the key if it's present, otherwise we don't want it in the config --- apmpackage/apm/agent/input/template.yml.hbs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apmpackage/apm/agent/input/template.yml.hbs b/apmpackage/apm/agent/input/template.yml.hbs index 62f822b8d5d..a76f35fd767 100644 --- a/apmpackage/apm/agent/input/template.yml.hbs +++ b/apmpackage/apm/agent/input/template.yml.hbs @@ -6,7 +6,9 @@ apm-server: default_service_environment: {{default_service_environment}} rum: enabled: {{enable_rum}} + {{#if sourcemap_api_key}} source_mapping.elasticsearch.api_key: {{sourcemap_api_key}} + {{/if}} allow_service_names: {{rum_allow_service_names}} allow_origins: {{rum_allow_origins}} allow_headers: {{rum_allow_headers}} From e86415667839aafc3934ffd07831035eea3b6f2c Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 16 Jun 2021 16:17:38 +0200 Subject: [PATCH 40/55] add tls + response decompression --- beater/beater.go | 57 ++++++++++++++++++++++++++--------- beater/beater_test.go | 6 ++-- sourcemap/fleet_store.go | 50 +++++++++++++++++++++++++----- sourcemap/fleet_store_test.go | 20 +++++++++--- sourcemap/store_test.go | 32 +++++++++++++++----- 5 files changed, 130 insertions(+), 35 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index 28868363c13..fa2cbc80e77 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -28,6 +28,8 @@ import ( "time" "github.com/elastic/beats/v7/libbeat/common/fleetmode" + "github.com/elastic/beats/v7/libbeat/common/transport" + "github.com/elastic/beats/v7/libbeat/common/transport/tlscommon" "github.com/pkg/errors" "go.elastic.co/apm" @@ -257,6 +259,7 @@ func (s *serverCreator) Create(p beat.PipelineConnector, rawConfig *common.Confi Namespace: namespace, Pipeline: p, RawConfig: apmServerCommonConfig, + FleetConfig: &integrationConfig.Fleet, }) } @@ -278,6 +281,7 @@ type serverRunner struct { acker *waitPublishedAcker namespace string config *config.Config + fleetConfig *config.Fleet beat *beat.Beat logger *logp.Logger tracer *apm.Tracer @@ -288,9 +292,10 @@ type serverRunner struct { type serverRunnerParams struct { sharedServerRunnerParams - Namespace string - Pipeline beat.PipelineConnector - RawConfig *common.Config + Namespace string + Pipeline beat.PipelineConnector + RawConfig *common.Config + FleetConfig *config.Fleet } type sharedServerRunnerParams struct { @@ -315,6 +320,7 @@ func newServerRunner(ctx context.Context, args serverRunnerParams) (*serverRunne cancelRunServerContext: cancel, config: cfg, + fleetConfig: args.FleetConfig, acker: args.Acker, pipeline: args.Pipeline, namespace: args.Namespace, @@ -353,8 +359,7 @@ func (s *serverRunner) Start() { func (s *serverRunner) run() error { // Send config to telemetry. recordAPMServerConfig(s.config) - - transformConfig, err := newTransformConfig(s.beat.Info, s.config) + transformConfig, err := newTransformConfig(s.beat.Info, s.config, s.fleetConfig) if err != nil { return err } @@ -596,7 +601,7 @@ func runServerWithTracerServer(runServer RunServerFunc, tracerServer *tracerServ } } -func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Config, error) { +func newTransformConfig(beatInfo beat.Info, cfg *config.Config, fleetCfg *config.Fleet) (*transform.Config, error) { transformConfig := &transform.Config{ DataStreams: cfg.DataStreams.Enabled, RUM: transform.RUMConfig{ @@ -606,7 +611,7 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf } if cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled { - store, err := newSourcemapStore(beatInfo, cfg.RumConfig.SourceMapping) + store, err := newSourcemapStore(beatInfo, cfg.RumConfig.SourceMapping, fleetCfg) if err != nil { return nil, err } @@ -616,15 +621,39 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config) (*transform.Conf return transformConfig, nil } -func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping) (*sourcemap.Store, error) { +func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping, fleetCfg *config.Fleet) (*sourcemap.Store, error) { if fleetmode.Enabled() { - // TODO: Configure the client with fleet TLS certs, timeouts, etc. - // Fleet hands TLS certs to the child processes during the gRPC - // handshake? - c := *http.DefaultClient + var ( + c = *http.DefaultClient + rt = http.DefaultTransport + ) + if fleetCfg != nil { + var tlsConfig *tlscommon.TLSConfig + var err error + if fleetCfg.TLS.IsEnabled() { + if tlsConfig, err = tlscommon.LoadTLSConfig(fleetCfg.TLS); err != nil { + return nil, err + } + } + + // Default for es is 90s :shrug: + timeout := 30 * time.Second + dialer := transport.NetDialer(timeout) + tlsDialer, err := transport.TLSDialer(dialer, tlsConfig, timeout) + if err != nil { + return nil, err + } + + rt = &http.Transport{ + Proxy: http.ProxyFromEnvironment, + Dial: dialer.Dial, + DialTLS: tlsDialer.Dial, + TLSClientConfig: tlsConfig.ToConfig(), + } + } - c.Transport = apmhttp.WrapRoundTripper(http.DefaultTransport) - return sourcemap.NewFleetStore(&c, cfg.ESConfig.APIKey, cfg.Metadata, cfg.Cache.Expiration) + c.Transport = apmhttp.WrapRoundTripper(rt) + return sourcemap.NewFleetStore(&c, fleetCfg, cfg.Metadata, cfg.Cache.Expiration) } c, err := elasticsearch.NewClient(cfg.ESConfig) if err != nil { diff --git a/beater/beater_test.go b/beater/beater_test.go index 55a5a9a02a9..7aade29c9f7 100644 --- a/beater/beater_test.go +++ b/beater/beater_test.go @@ -248,7 +248,7 @@ func TestTransformConfigIndex(t *testing.T) { cfg.RumConfig.SourceMapping.IndexPattern = indexPattern } - transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg) + transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg, nil) require.NoError(t, err) require.NotNil(t, transformConfig.RUM.SourcemapStore) transformConfig.RUM.SourcemapStore.Added(context.Background(), "name", "version", "path") @@ -268,7 +268,7 @@ func TestTransformConfig(t *testing.T) { cfg := config.DefaultConfig() cfg.RumConfig.Enabled = rumEnabled cfg.RumConfig.SourceMapping.Enabled = sourcemapEnabled - transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg) + transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg, nil) require.NoError(t, err) if expectSourcemapStore { assert.NotNil(t, transformConfig.RUM.SourcemapStore) @@ -297,7 +297,7 @@ func TestStoreUsesRUMElasticsearchConfig(t *testing.T) { cfg.RumConfig.SourceMapping.ESConfig = elasticsearch.DefaultConfig() cfg.RumConfig.SourceMapping.ESConfig.Hosts = []string{ts.URL} - transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg) + transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg, nil) require.NoError(t, err) // Check that the provided rum elasticsearch config was used and // Fetch() goes to the test server. diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 7c7003f31b0..637c111df07 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -19,6 +19,7 @@ package sourcemap import ( "bytes" + "compress/zlib" "context" "fmt" "io" @@ -26,6 +27,9 @@ import ( "net/http" "time" + "github.com/pkg/errors" + + "github.com/elastic/beats/v7/libbeat/common" "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/beater/config" @@ -48,26 +52,45 @@ type key struct { // stored in Fleet-Server. func NewFleetStore( c *http.Client, - apikey string, + fleetCfg *config.Fleet, cfgs []config.SourceMapMetadata, expiration time.Duration, ) (*Store, error) { + if len(fleetCfg.Hosts) < 1 { + return nil, errors.New("no fleet hosts present for fleet store") + } logger := logp.NewLogger(logs.Sourcemap) - s := newFleetStore(c, apikey, cfgs) + s, err := newFleetStore(c, fleetCfg, cfgs) + if err != nil { + return nil, err + } return newStore(s, logger, expiration) } -func newFleetStore(c *http.Client, apikey string, cfgs []config.SourceMapMetadata) fleetStore { +func newFleetStore( + c *http.Client, + fleetCfg *config.Fleet, + cfgs []config.SourceMapMetadata, +) (fleetStore, error) { + // TODO(stn): This is assuming we'll get a single fleet address right now. + // If we get more, how do we know which artifact is where? Are they + // present on all fleet-servers and we should rr through the addresses? + host := fleetCfg.Hosts[0] fleetURLs := make(map[key]string) + for _, cfg := range cfgs { k := key{cfg.ServiceName, cfg.ServiceVersion, cfg.BundleFilepath} - fleetURLs[k] = cfg.SourceMapURL + u, err := common.MakeURL(fleetCfg.Protocol, cfg.SourceMapURL, host, 8220) + if err != nil { + return fleetStore{}, err + } + fleetURLs[k] = u } return fleetStore{ - apikey: "ApiKey " + apikey, + apikey: "ApiKey " + fleetCfg.AccessAPIKey, fleetURLs: fleetURLs, c: c, - } + }, nil } func (f fleetStore) fetch(ctx context.Context, name, version, path string) (string, error) { @@ -78,6 +101,7 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri name, version, path, ) } + req, err := http.NewRequest(http.MethodGet, fleetURL, nil) if err != nil { return "", err @@ -99,9 +123,21 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri return "", fmt.Errorf("failure querying fleet: statuscode=%d response=%s", resp.StatusCode, body) } + // Do the headers show compression??? + buf := new(bytes.Buffer) - if _, err := io.Copy(buf, resp.Body); err != nil { + // TODO: Caue said that the response is b64 encoded. + // body := base64.NewDecoder(base64.StdEncoding, resp.Body) + // Looking at the index in elasticsearch, currently + // - no encryption + // - zlib compression + r, err := zlib.NewReader(resp.Body) + if err != nil { + return "", err + } + + if _, err := io.Copy(buf, r); err != nil { return "", err } diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go index 09ad1e7398f..39dada12e76 100644 --- a/sourcemap/fleet_store_test.go +++ b/sourcemap/fleet_store_test.go @@ -18,6 +18,7 @@ package sourcemap import ( + "compress/zlib" "context" "net/http" "net/http/httptest" @@ -28,7 +29,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestFetch(t *testing.T) { +func TestFleetFetch(t *testing.T) { var ( hasAuth bool apikey = "supersecret" @@ -42,19 +43,30 @@ func TestFetch(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { auth := r.Header.Get("Authorization") hasAuth = auth == "ApiKey "+apikey - w.Write([]byte(wantRes)) + // zlib compress + wr := zlib.NewWriter(w) + defer wr.Close() + wr.Write([]byte(wantRes)) })) defer ts.Close() + fleetCfg := &config.Fleet{ + Hosts: []string{ts.URL}, + Protocol: "https", + AccessAPIKey: apikey, + TLS: nil, + } + cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, BundleFilepath: path, - SourceMapURL: ts.URL, + SourceMapURL: "/", }, } - fb := newFleetStore(c, apikey, cfgs) + fb, err := newFleetStore(c, fleetCfg, cfgs) + assert.NoError(t, err) gotRes, err := fb.fetch(context.Background(), name, version, path) assert.NoError(t, err) diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index b2a9ad274a7..4a5eaf7a46d 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -18,6 +18,7 @@ package sourcemap import ( + "compress/zlib" "context" "errors" "fmt" @@ -165,19 +166,29 @@ func TestFetchTimeout(t *testing.T) { ) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { <-waitc - w.Write([]byte(test.ValidSourcemap)) + // zlib compress + wr := zlib.NewWriter(w) + defer wr.Close() + wr.Write([]byte(test.ValidSourcemap)) })) defer ts.Close() + fleetCfg := &config.Fleet{ + Hosts: []string{ts.URL}, + Protocol: "https", + AccessAPIKey: apikey, + TLS: nil, + } cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, BundleFilepath: path, - SourceMapURL: ts.URL, + SourceMapURL: "", }, } - b := newFleetStore(c, apikey, cfgs) + b, err := newFleetStore(c, fleetCfg, cfgs) + assert.NoError(t, err) logger := logp.NewLogger(logs.Sourcemap) store, err := newStore(b, logger, time.Minute) assert.NoError(t, err) @@ -191,7 +202,6 @@ func TestFetchTimeout(t *testing.T) { go func() { consumer, err := store.Fetch(ctx, name, version, path) if err != nil { - assert.True(t, errors.Is(err, context.DeadlineExceeded)) atomic.AddInt64(&errs, 1) close(waitc) @@ -236,19 +246,27 @@ func TestConcurrentFetch(t *testing.T) { http.Error(w, "err", http.StatusInternalServerError) return } - w.Write([]byte(test.ValidSourcemap)) + wr := zlib.NewWriter(w) + defer wr.Close() + wr.Write([]byte(test.ValidSourcemap)) })) defer ts.Close() + fleetCfg := &config.Fleet{ + Hosts: []string{ts.URL}, + Protocol: "https", + AccessAPIKey: apikey, + TLS: nil, + } cfgs := []config.SourceMapMetadata{ { ServiceName: name, ServiceVersion: version, BundleFilepath: path, - SourceMapURL: ts.URL, + SourceMapURL: "", }, } - store, err := NewFleetStore(c, apikey, cfgs, time.Minute) + store, err := NewFleetStore(c, fleetCfg, cfgs, time.Minute) assert.NoError(t, err) var wg sync.WaitGroup From 5b3ba88ab252c0687445b8352474d6effe314f2b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 16 Jun 2021 16:27:41 +0200 Subject: [PATCH 41/55] remove dead comments and move code --- sourcemap/fleet_store.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index 637c111df07..c7273fb3cf8 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -123,12 +123,6 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri return "", fmt.Errorf("failure querying fleet: statuscode=%d response=%s", resp.StatusCode, body) } - // Do the headers show compression??? - - buf := new(bytes.Buffer) - - // TODO: Caue said that the response is b64 encoded. - // body := base64.NewDecoder(base64.StdEncoding, resp.Body) // Looking at the index in elasticsearch, currently // - no encryption // - zlib compression @@ -137,6 +131,7 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri return "", err } + buf := new(bytes.Buffer) if _, err := io.Copy(buf, r); err != nil { return "", err } From db22ff0e46ac1af093daffb2f22c1c265877b219 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 10:36:46 +0200 Subject: [PATCH 42/55] Update apmpackage/apm/agent/input/template.yml.hbs Co-authored-by: Andrew Wilkins --- apmpackage/apm/agent/input/template.yml.hbs | 3 --- 1 file changed, 3 deletions(-) diff --git a/apmpackage/apm/agent/input/template.yml.hbs b/apmpackage/apm/agent/input/template.yml.hbs index aab4c4d56bd..138867e1589 100644 --- a/apmpackage/apm/agent/input/template.yml.hbs +++ b/apmpackage/apm/agent/input/template.yml.hbs @@ -14,9 +14,6 @@ apm-server: response_headers: {{response_headers}} rum: enabled: {{enable_rum}} - {{#if sourcemap_api_key}} - source_mapping.elasticsearch.api_key: {{sourcemap_api_key}} - {{/if}} allow_headers: {{#each rum_allow_headers}} - {{this}} From 778d0e93ee96724bba2b08870623236ccd66f3d7 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Sat, 19 Jun 2021 10:58:03 +0800 Subject: [PATCH 43/55] Move rate limiting and service name restrictions out of processor/stream (#5492) * Move rate-limiting out of processor/stream ... and into beater/api/intake, injected into the stream processor as a batch processor. This paves the way for taking a similar approach for otel. * processor/stream: unexport some things Unexport some things that are no longer needed for tests. * Move allowed service names out of processor/stream ... and into beater/api, injected into the stream processor as a batch processor. This paves the way for taking a similar approach for otel. * beater/api/intake: revise rate-limiting Rate limit after reading but before processing. Simpler and in line with what we will do for OTel. * Remove stale comment * Reinstate initial Allow * beater/api/intake: add missing return --- beater/api/intake/handler.go | 59 ++++++++-- beater/api/intake/handler_test.go | 80 ++++++++++--- .../LimiterAllowAll.approved.json | 3 + .../LimiterDeny.approved.json | 8 ++ .../LimiterDenyAll.approved.json} | 0 ...miterPartiallyUsedLimitAllow.approved.json | 3 + ...imiterPartiallyUsedLimitDeny.approved.json | 8 ++ beater/api/mux.go | 43 +++++-- beater/api/mux_intake_rum_test.go | 35 ++++++ processor/stream/benchmark_test.go | 43 +++---- processor/stream/processor.go | 102 ++++------------- processor/stream/processor_test.go | 107 +----------------- 12 files changed, 246 insertions(+), 245 deletions(-) create mode 100644 beater/api/intake/test_approved/TestRateLimiting/LimiterAllowAll.approved.json create mode 100644 beater/api/intake/test_approved/TestRateLimiting/LimiterDeny.approved.json rename beater/api/intake/test_approved/{RateLimit.approved.json => TestRateLimiting/LimiterDenyAll.approved.json} (100%) create mode 100644 beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitAllow.approved.json create mode 100644 beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitDeny.approved.json diff --git a/beater/api/intake/handler.go b/beater/api/intake/handler.go index 1bffb4421a8..2bfb47b7037 100644 --- a/beater/api/intake/handler.go +++ b/beater/api/intake/handler.go @@ -24,6 +24,7 @@ import ( "io" "net/http" "strings" + "time" "golang.org/x/time/rate" @@ -33,10 +34,16 @@ import ( "github.com/elastic/apm-server/beater/request" "github.com/elastic/apm-server/decoder" "github.com/elastic/apm-server/model" + "github.com/elastic/apm-server/model/modelprocessor" "github.com/elastic/apm-server/processor/stream" "github.com/elastic/apm-server/publish" ) +const ( + batchSize = 10 + rateLimitTimeout = time.Second +) + var ( // MonitoringMap holds a mapping for request.IDs to monitoring counters MonitoringMap = request.DefaultMonitoringMapForRegistry(registry) @@ -45,34 +52,45 @@ var ( errMethodNotAllowed = errors.New("only POST requests are supported") errServerShuttingDown = errors.New("server is shutting down") errInvalidContentType = errors.New("invalid content type") - errRateLimitExceeded = stream.ErrRateLimitExceeded + errRateLimitExceeded = errors.New("rate limit exceeded") ) // StreamHandler is an interface for handling an Elastic APM agent ND-JSON event // stream, implemented by processor/stream. type StreamHandler interface { HandleStream( - context.Context, - *rate.Limiter, - *model.Metadata, - io.Reader, - model.BatchProcessor, - *stream.Result, + ctx context.Context, + meta *model.Metadata, + stream io.Reader, + batchSize int, + processor model.BatchProcessor, + out *stream.Result, ) error } // Handler returns a request.Handler for managing intake requests for backend and rum events. func Handler(handler StreamHandler, batchProcessor model.BatchProcessor) request.Handler { return func(c *request.Context) { - if err := validateRequest(c); err != nil { writeError(c, err) return } - if c.RateLimiter != nil && !c.RateLimiter.Allow() { - writeError(c, errRateLimitExceeded) - return + if c.RateLimiter != nil { + // Call Allow once for each stream to avoid burning CPU before we + // begin reading for the first time. This prevents clients from + // repeatedly connecting and sending < batchSize events and + // disconnecting before being rate limited. + if !c.RateLimiter.Allow() { + writeError(c, errRateLimitExceeded) + return + } + + // Apply rate limiting after reading but before processing any events. + batchProcessor = modelprocessor.Chained{ + rateLimitBatchProcessor(c.RateLimiter, batchSize), + batchProcessor, + } } reader, err := decoder.CompressedRequestReader(c.Request) @@ -90,9 +108,9 @@ func Handler(handler StreamHandler, batchProcessor model.BatchProcessor) request var result stream.Result if err := handler.HandleStream( c.Request.Context(), - c.RateLimiter, &metadata, reader, + batchSize, batchProcessor, &result, ); err != nil { @@ -102,6 +120,23 @@ func Handler(handler StreamHandler, batchProcessor model.BatchProcessor) request } } +func rateLimitBatchProcessor(limiter *rate.Limiter, batchSize int) model.ProcessBatchFunc { + return func(ctx context.Context, _ *model.Batch) error { + return rateLimitBatch(ctx, limiter, batchSize) + } +} + +// rateLimitBatch waits up to one second for the rate limiter to allow +// batchSize events to be read and processed. +func rateLimitBatch(ctx context.Context, limiter *rate.Limiter, batchSize int) error { + ctx, cancel := context.WithTimeout(ctx, rateLimitTimeout) + defer cancel() + if err := limiter.WaitN(ctx, batchSize); err != nil { + return errRateLimitExceeded + } + return nil +} + func validateRequest(c *request.Context) error { if c.Request.Method != http.MethodPost { return errMethodNotAllowed diff --git a/beater/api/intake/handler_test.go b/beater/api/intake/handler_test.go index 61923c0bf92..5a4b7cf1b4d 100644 --- a/beater/api/intake/handler_test.go +++ b/beater/api/intake/handler_test.go @@ -27,24 +27,23 @@ import ( "net/http/httptest" "path/filepath" "testing" - - "github.com/elastic/apm-server/approvaltest" - "github.com/elastic/apm-server/beater/api/ratelimit" - "github.com/elastic/apm-server/model" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/time/rate" + "github.com/elastic/apm-server/approvaltest" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/headers" "github.com/elastic/apm-server/beater/request" + "github.com/elastic/apm-server/model" + "github.com/elastic/apm-server/model/modelprocessor" "github.com/elastic/apm-server/processor/stream" "github.com/elastic/apm-server/publish" ) func TestIntakeHandler(t *testing.T) { - var rateLimit, err = ratelimit.NewStore(1, 0, 0) - require.NoError(t, err) for name, tc := range map[string]testcaseIntakeHandler{ "Method": { path: "errors.ndjson", @@ -60,11 +59,6 @@ func TestIntakeHandler(t *testing.T) { }(), code: http.StatusBadRequest, id: request.IDResponseErrorsValidate, }, - "RateLimit": { - path: "errors.ndjson", - rateLimit: rateLimit, - code: http.StatusTooManyRequests, id: request.IDResponseErrorsRateLimit, - }, "BodyReader": { path: "errors.ndjson", r: func() *http.Request { @@ -141,9 +135,6 @@ func TestIntakeHandler(t *testing.T) { // setup tc.setup(t) - if tc.rateLimit != nil { - tc.c.RateLimiter = tc.rateLimit.ForIP(&http.Request{}) - } // call handler h := Handler(tc.processor, tc.batchProcessor) h(tc.c) @@ -164,12 +155,69 @@ func TestIntakeHandler(t *testing.T) { } } +func TestRateLimiting(t *testing.T) { + type test struct { + limiter *rate.Limiter + preconsumed int + expectLimited bool + } + + for name, test := range map[string]test{ + "LimiterAllowAll": { + limiter: rate.NewLimiter(rate.Limit(40), 40*5), + expectLimited: false, + }, + "LimiterPartiallyUsedLimitAllow": { + limiter: rate.NewLimiter(rate.Limit(10), 10*2), + preconsumed: 10, + expectLimited: false, + }, + "LimiterDenyAll": { + limiter: rate.NewLimiter(rate.Limit(0), 2), + expectLimited: true, + }, + "LimiterPartiallyUsedLimitDeny": { + limiter: rate.NewLimiter(rate.Limit(7), 7*2), + preconsumed: 10, + expectLimited: true, + }, + "LimiterDeny": { + limiter: rate.NewLimiter(rate.Limit(6), 6*2), + expectLimited: true, + }, + } { + t.Run(name, func(t *testing.T) { + var tc testcaseIntakeHandler + tc.path = "ratelimit.ndjson" + tc.setup(t) + tc.c.RateLimiter = test.limiter + if test.preconsumed > 0 { + test.limiter.AllowN(time.Now(), test.preconsumed) + } + + h := Handler(tc.processor, tc.batchProcessor) + h(tc.c) + + if test.expectLimited { + assert.Equal(t, request.IDResponseErrorsRateLimit, tc.c.Result.ID) + assert.Equal(t, http.StatusTooManyRequests, tc.w.Code) + assert.Error(t, tc.c.Result.Err) + } else { + assert.Equal(t, request.IDResponseValidAccepted, tc.c.Result.ID) + assert.Equal(t, http.StatusAccepted, tc.w.Code) + assert.NoError(t, tc.c.Result.Err) + } + assert.NotZero(t, tc.w.Body.Len()) + approvaltest.ApproveJSON(t, "test_approved/"+t.Name(), tc.w.Body.Bytes()) + }) + } +} + type testcaseIntakeHandler struct { c *request.Context w *httptest.ResponseRecorder r *http.Request processor *stream.Processor - rateLimit *ratelimit.Store batchProcessor model.BatchProcessor path string @@ -183,7 +231,7 @@ func (tc *testcaseIntakeHandler) setup(t *testing.T) { tc.processor = stream.BackendProcessor(cfg) } if tc.batchProcessor == nil { - tc.batchProcessor = model.ProcessBatchFunc(func(context.Context, *model.Batch) error { return nil }) + tc.batchProcessor = modelprocessor.Nop{} } if tc.r == nil { diff --git a/beater/api/intake/test_approved/TestRateLimiting/LimiterAllowAll.approved.json b/beater/api/intake/test_approved/TestRateLimiting/LimiterAllowAll.approved.json new file mode 100644 index 00000000000..c612a4faf20 --- /dev/null +++ b/beater/api/intake/test_approved/TestRateLimiting/LimiterAllowAll.approved.json @@ -0,0 +1,3 @@ +{ + "accepted": 19 +} diff --git a/beater/api/intake/test_approved/TestRateLimiting/LimiterDeny.approved.json b/beater/api/intake/test_approved/TestRateLimiting/LimiterDeny.approved.json new file mode 100644 index 00000000000..8333c5a198a --- /dev/null +++ b/beater/api/intake/test_approved/TestRateLimiting/LimiterDeny.approved.json @@ -0,0 +1,8 @@ +{ + "accepted": 10, + "errors": [ + { + "message": "rate limit exceeded" + } + ] +} diff --git a/beater/api/intake/test_approved/RateLimit.approved.json b/beater/api/intake/test_approved/TestRateLimiting/LimiterDenyAll.approved.json similarity index 100% rename from beater/api/intake/test_approved/RateLimit.approved.json rename to beater/api/intake/test_approved/TestRateLimiting/LimiterDenyAll.approved.json diff --git a/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitAllow.approved.json b/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitAllow.approved.json new file mode 100644 index 00000000000..c612a4faf20 --- /dev/null +++ b/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitAllow.approved.json @@ -0,0 +1,3 @@ +{ + "accepted": 19 +} diff --git a/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitDeny.approved.json b/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitDeny.approved.json new file mode 100644 index 00000000000..8333c5a198a --- /dev/null +++ b/beater/api/intake/test_approved/TestRateLimiting/LimiterPartiallyUsedLimitDeny.approved.json @@ -0,0 +1,8 @@ +{ + "accepted": 10, + "errors": [ + { + "message": "rate limit exceeded" + } + ] +} diff --git a/beater/api/mux.go b/beater/api/mux.go index ef6e53afe69..55e9c1ee73f 100644 --- a/beater/api/mux.go +++ b/beater/api/mux.go @@ -18,6 +18,7 @@ package api import ( + "context" "net/http" "net/http/pprof" @@ -37,6 +38,7 @@ import ( "github.com/elastic/apm-server/beater/request" logs "github.com/elastic/apm-server/log" "github.com/elastic/apm-server/model" + "github.com/elastic/apm-server/model/modelprocessor" "github.com/elastic/apm-server/processor/stream" "github.com/elastic/apm-server/publish" ) @@ -100,8 +102,8 @@ func NewMux( {AssetSourcemapPath, builder.sourcemapHandler}, {AgentConfigPath, builder.backendAgentConfigHandler(fetcher)}, {AgentConfigRUMPath, builder.rumAgentConfigHandler(fetcher)}, - {IntakeRUMPath, builder.rumIntakeHandler}, - {IntakeRUMV3Path, builder.rumV3IntakeHandler}, + {IntakeRUMPath, builder.rumIntakeHandler(stream.RUMV2Processor)}, + {IntakeRUMV3Path, builder.rumIntakeHandler(stream.RUMV3Processor)}, {IntakePath, builder.backendIntakeHandler}, // The profile endpoint is in Beta {ProfilePath, builder.profileHandler}, @@ -152,14 +154,13 @@ func (r *routeBuilder) backendIntakeHandler() (request.Handler, error) { return middleware.Wrap(h, backendMiddleware(r.cfg, authHandler, intake.MonitoringMap)...) } -func (r *routeBuilder) rumIntakeHandler() (request.Handler, error) { - h := intake.Handler(stream.RUMV2Processor(r.cfg), r.batchProcessor) - return middleware.Wrap(h, rumMiddleware(r.cfg, nil, intake.MonitoringMap)...) -} - -func (r *routeBuilder) rumV3IntakeHandler() (request.Handler, error) { - h := intake.Handler(stream.RUMV3Processor(r.cfg), r.batchProcessor) - return middleware.Wrap(h, rumMiddleware(r.cfg, nil, intake.MonitoringMap)...) +func (r *routeBuilder) rumIntakeHandler(newProcessor func(*config.Config) *stream.Processor) func() (request.Handler, error) { + return func() (request.Handler, error) { + batchProcessor := r.batchProcessor + batchProcessor = batchProcessorWithAllowedServiceNames(batchProcessor, r.cfg.RumConfig.AllowServiceNames) + h := intake.Handler(newProcessor(r.cfg), batchProcessor) + return middleware.Wrap(h, rumMiddleware(r.cfg, nil, intake.MonitoringMap)...) + } } func (r *routeBuilder) sourcemapHandler() (request.Handler, error) { @@ -265,3 +266,25 @@ func rootMiddleware(cfg *config.Config, auth *authorization.Handler) []middlewar middleware.ResponseHeadersMiddleware(cfg.ResponseHeaders), middleware.AuthorizationMiddleware(auth, false)) } + +// TODO(axw) move this into the authorization package when introducing anonymous auth. +func batchProcessorWithAllowedServiceNames(p model.BatchProcessor, allowedServiceNames []string) model.BatchProcessor { + if len(allowedServiceNames) == 0 { + // All service names are allowed. + return p + } + m := make(map[string]bool) + for _, name := range allowedServiceNames { + m[name] = true + } + var restrictServiceName modelprocessor.MetadataProcessorFunc = func(ctx context.Context, meta *model.Metadata) error { + // Restrict to explicitly allowed service names. + // The list of allowed service names is not considered secret, + // so we do not use constant time comparison. + if !m[meta.Service.Name] { + return &stream.InvalidInputError{Message: "service name is not allowed"} + } + return nil + } + return modelprocessor.Chained{restrictServiceName, p} +} diff --git a/beater/api/mux_intake_rum_test.go b/beater/api/mux_intake_rum_test.go index c1d35d25548..5bf99c1ee44 100644 --- a/beater/api/mux_intake_rum_test.go +++ b/beater/api/mux_intake_rum_test.go @@ -18,6 +18,8 @@ package api import ( + "bytes" + "io/ioutil" "net/http" "net/http/httptest" "testing" @@ -121,6 +123,39 @@ func TestRumHandler_MonitoringMiddleware(t *testing.T) { } } +func TestRUMHandler_AllowedServiceNames(t *testing.T) { + payload, err := ioutil.ReadFile("../../testdata/intake-v2/transactions_spans_rum.ndjson") + require.NoError(t, err) + + for _, test := range []struct { + AllowServiceNames []string + Allowed bool + }{{ + AllowServiceNames: nil, + Allowed: true, // none specified = all allowed + }, { + AllowServiceNames: []string{"apm-agent-js"}, // matches what's in test data + Allowed: true, + }, { + AllowServiceNames: []string{"reject_everything"}, + Allowed: false, + }} { + cfg := cfgEnabledRUM() + cfg.RumConfig.AllowServiceNames = test.AllowServiceNames + h := newTestMux(t, cfg) + + req := httptest.NewRequest(http.MethodPost, "/intake/v2/rum/events", bytes.NewReader(payload)) + req.Header.Set("Content-Type", "application/x-ndjson") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if test.Allowed { + assert.Equal(t, http.StatusAccepted, w.Code) + } else { + assert.Equal(t, http.StatusBadRequest, w.Code) + } + } +} + func cfgEnabledRUM() *config.Config { cfg := config.DefaultConfig() cfg.RumConfig.Enabled = true diff --git a/processor/stream/benchmark_test.go b/processor/stream/benchmark_test.go index ef1df774f1a..1ec5ba7ea53 100644 --- a/processor/stream/benchmark_test.go +++ b/processor/stream/benchmark_test.go @@ -21,12 +21,9 @@ import ( "bytes" "context" "io/ioutil" - "math" "path/filepath" "testing" - "golang.org/x/time/rate" - "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/model" ) @@ -44,36 +41,30 @@ func BenchmarkRUMV3Processor(b *testing.B) { } func benchmarkStreamProcessor(b *testing.B, processor *Processor, files []string) { + const batchSize = 10 batchProcessor := nopBatchProcessor{} + benchmark := func(b *testing.B, filename string) { + data, err := ioutil.ReadFile(filename) + if err != nil { + b.Error(err) + } + r := bytes.NewReader(data) + b.ReportAllocs() + b.SetBytes(int64(len(data))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + b.StopTimer() + r.Reset(data) + b.StartTimer() - //ensure to not hit rate limit as blocking wait would be measured otherwise - rl := rate.NewLimiter(rate.Limit(math.MaxFloat64-1), math.MaxInt32) - - benchmark := func(filename string, rl *rate.Limiter) func(b *testing.B) { - return func(b *testing.B) { - data, err := ioutil.ReadFile(filename) - if err != nil { - b.Error(err) - } - r := bytes.NewReader(data) - b.ReportAllocs() - b.SetBytes(int64(len(data))) - b.ResetTimer() - for i := 0; i < b.N; i++ { - b.StopTimer() - r.Reset(data) - b.StartTimer() - - var result Result - processor.HandleStream(context.Background(), rl, &model.Metadata{}, r, batchProcessor, &result) - } + var result Result + processor.HandleStream(context.Background(), &model.Metadata{}, r, batchSize, batchProcessor, &result) } } for _, f := range files { b.Run(filepath.Base(f), func(b *testing.B) { - b.Run("NoRateLimit", benchmark(f, nil)) - b.Run("WithRateLimit", benchmark(f, rl)) + benchmark(b, f) }) } } diff --git a/processor/stream/processor.go b/processor/stream/processor.go index 0fdfb2c45ea..bb01e3ce44f 100644 --- a/processor/stream/processor.go +++ b/processor/stream/processor.go @@ -24,8 +24,6 @@ import ( "sync" "time" - "golang.org/x/time/rate" - "go.elastic.co/apm" "github.com/pkg/errors" @@ -36,19 +34,14 @@ import ( "github.com/elastic/apm-server/model/modeldecoder" "github.com/elastic/apm-server/model/modeldecoder/rumv3" v2 "github.com/elastic/apm-server/model/modeldecoder/v2" - "github.com/elastic/apm-server/model/modelprocessor" "github.com/elastic/apm-server/utility" ) var ( - ErrUnrecognizedObject = errors.New("did not recognize object type") - - ErrRateLimitExceeded = errors.New("rate limit exceeded") + errUnrecognizedObject = errors.New("did not recognize object type") ) const ( - batchSize = 10 - errorEventType = "error" metricsetEventType = "metricset" spanEventType = "span" @@ -61,12 +54,11 @@ const ( type decodeMetadataFunc func(decoder.Decoder, *model.Metadata) error type Processor struct { - Mconfig modeldecoder.Config - MaxEventSize int - streamReaderPool sync.Pool - decodeMetadata decodeMetadataFunc - isRUM bool - allowedServiceNames map[string]bool + Mconfig modeldecoder.Config + MaxEventSize int + streamReaderPool sync.Pool + decodeMetadata decodeMetadataFunc + isRUM bool } func BackendProcessor(cfg *config.Config) *Processor { @@ -80,33 +72,20 @@ func BackendProcessor(cfg *config.Config) *Processor { func RUMV2Processor(cfg *config.Config) *Processor { return &Processor{ - Mconfig: modeldecoder.Config{Experimental: cfg.Mode == config.ModeExperimental}, - MaxEventSize: cfg.MaxEventSize, - decodeMetadata: v2.DecodeNestedMetadata, - isRUM: true, - allowedServiceNames: makeAllowedServiceNamesMap(cfg.RumConfig.AllowServiceNames), + Mconfig: modeldecoder.Config{Experimental: cfg.Mode == config.ModeExperimental}, + MaxEventSize: cfg.MaxEventSize, + decodeMetadata: v2.DecodeNestedMetadata, + isRUM: true, } } func RUMV3Processor(cfg *config.Config) *Processor { return &Processor{ - Mconfig: modeldecoder.Config{Experimental: cfg.Mode == config.ModeExperimental}, - MaxEventSize: cfg.MaxEventSize, - decodeMetadata: rumv3.DecodeNestedMetadata, - isRUM: true, - allowedServiceNames: makeAllowedServiceNamesMap(cfg.RumConfig.AllowServiceNames), - } -} - -func makeAllowedServiceNamesMap(allowed []string) map[string]bool { - if len(allowed) == 0 { - return nil - } - m := make(map[string]bool, len(allowed)) - for _, name := range allowed { - m[name] = true + Mconfig: modeldecoder.Config{Experimental: cfg.Mode == config.ModeExperimental}, + MaxEventSize: cfg.MaxEventSize, + decodeMetadata: rumv3.DecodeNestedMetadata, + isRUM: true, } - return m } func (p *Processor) readMetadata(reader *streamReader, metadata *model.Metadata) error { @@ -129,17 +108,13 @@ func (p *Processor) readMetadata(reader *streamReader, metadata *model.Metadata) return nil } -// IdentifyEventType takes a reader and reads ahead the first key of the +// identifyEventType takes a reader and reads ahead the first key of the // underlying json input. This method makes some assumptions met by the // input format: // - the input is in JSON format // - every valid ndjson line only has one root key // - the bytes that we must match on are ASCII -// -// NOTE(axw) this method really should not be exported, but it has to be -// for package_test. When we migrate that code to system tests, unexport -// this method. -func (p *Processor) IdentifyEventType(body []byte) []byte { +func (p *Processor) identifyEventType(body []byte) []byte { // find event type, trim spaces and account for single and double quotes var quote byte var key []byte @@ -163,7 +138,6 @@ func (p *Processor) IdentifyEventType(body []byte) []byte { // the error err. func (p *Processor) readBatch( ctx context.Context, - ipRateLimiter *rate.Limiter, requestTime time.Time, streamMetadata *model.Metadata, batchSize int, @@ -172,19 +146,6 @@ func (p *Processor) readBatch( result *Result, ) (int, error) { - if ipRateLimiter != nil { - // TODO(axw) move this logic somewhere central, so it - // can be used by processor/otel too. - // - // use provided rate limiter to throttle batch read - ctxT, cancel := context.WithTimeout(ctx, time.Second) - err := ipRateLimiter.WaitN(ctxT, batchSize) - cancel() - if err != nil { - return 0, ErrRateLimitExceeded - } - } - // input events are decoded and appended to the batch var n int for i := 0; i < batchSize && !reader.IsEOF(); i++ { @@ -208,7 +169,7 @@ func (p *Processor) readBatch( Metadata: *streamMetadata, Config: p.Mconfig, } - switch eventType := p.IdentifyEventType(body); string(eventType) { + switch eventType := p.identifyEventType(body); string(eventType) { case errorEventType: var event model.Error err := v2.DecodeNestedError(reader, &input, &event) @@ -275,7 +236,7 @@ func (p *Processor) readBatch( n += 1 + len(event.Metricsets) + len(event.Spans) default: result.LimitedAdd(&InvalidInputError{ - Message: errors.Wrap(ErrUnrecognizedObject, string(eventType)).Error(), + Message: errors.Wrap(errUnrecognizedObject, string(eventType)).Error(), Document: string(reader.LatestLine()), }) continue @@ -298,8 +259,8 @@ func handleDecodeErr(err error, r *streamReader, result *Result) bool { return true } -// HandleStream processes a stream of events, updating result as events are -// accepted, or per-event errors occur. +// HandleStream processes a stream of events in batches of batchSize at a time, +// updating result as events are accepted, or per-event errors occur. // // HandleStream will return an error when a terminal stream-level error occurs, // such as the rate limit being exceeded, or due to authorization errors. In @@ -308,9 +269,9 @@ func handleDecodeErr(err error, r *streamReader, result *Result) bool { // Callers must not access result concurrently with HandleStream. func (p *Processor) HandleStream( ctx context.Context, - ipRateLimiter *rate.Limiter, meta *model.Metadata, reader io.Reader, + batchSize int, processor model.BatchProcessor, result *Result, ) error { @@ -323,10 +284,6 @@ func (p *Processor) HandleStream( return err } - var allowedServiceNamesProcessor model.BatchProcessor = modelprocessor.Nop{} - if p.allowedServiceNames != nil { - allowedServiceNamesProcessor = modelprocessor.MetadataProcessorFunc(p.restrictAllowedServiceNames) - } requestTime := utility.RequestTime(ctx) sp, ctx := apm.StartSpan(ctx, "Stream", "Reporter") @@ -334,11 +291,8 @@ func (p *Processor) HandleStream( for { var batch model.Batch - n, readErr := p.readBatch(ctx, ipRateLimiter, requestTime, meta, batchSize, &batch, sr, result) + n, readErr := p.readBatch(ctx, requestTime, meta, batchSize, &batch, sr, result) if n > 0 { - if err := allowedServiceNamesProcessor.ProcessBatch(ctx, &batch); err != nil { - return err - } // NOTE(axw) ProcessBatch takes ownership of batch, which means we cannot reuse // the slice memory. We should investigate alternative interfaces between the // processor and publisher which would enable better memory reuse, e.g. by using @@ -358,18 +312,6 @@ func (p *Processor) HandleStream( return nil } -func (p *Processor) restrictAllowedServiceNames(ctx context.Context, meta *model.Metadata) error { - // Restrict to explicitly allowed service names. The list of - // allowed service names is not considered secret, so we do - // not use constant time comparison. - if !p.allowedServiceNames[meta.Service.Name] { - return &InvalidInputError{ - Message: "service name is not allowed", - } - } - return nil -} - // getStreamReader returns a streamReader that reads ND-JSON lines from r. func (p *Processor) getStreamReader(r io.Reader) *streamReader { if sr, ok := p.streamReaderPool.Get().(*streamReader); ok { diff --git a/processor/stream/processor_test.go b/processor/stream/processor_test.go index cd2a93ead6c..8eb5b109419 100644 --- a/processor/stream/processor_test.go +++ b/processor/stream/processor_test.go @@ -30,13 +30,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "golang.org/x/time/rate" "github.com/elastic/apm-server/approvaltest" "github.com/elastic/apm-server/beater/beatertest" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/model" - "github.com/elastic/apm-server/model/modelprocessor" "github.com/elastic/apm-server/publish" "github.com/elastic/apm-server/transform" "github.com/elastic/apm-server/utility" @@ -57,7 +55,7 @@ func TestHandlerReadStreamError(t *testing.T) { sp := BackendProcessor(&config.Config{MaxEventSize: 100 * 1024}) var actualResult Result - err = sp.HandleStream(context.Background(), nil, &model.Metadata{}, timeoutReader, processor, &actualResult) + err = sp.HandleStream(context.Background(), &model.Metadata{}, timeoutReader, 10, processor, &actualResult) assert.EqualError(t, err, "timeout") assert.Equal(t, Result{Accepted: accepted}, actualResult) } @@ -83,8 +81,8 @@ func TestHandlerReportingStreamError(t *testing.T) { var actualResult Result err := sp.HandleStream( - context.Background(), nil, &model.Metadata{}, - bytes.NewReader(payload), processor, &actualResult, + context.Background(), &model.Metadata{}, + bytes.NewReader(payload), 10, processor, &actualResult, ) assert.Equal(t, test.err, err) assert.Zero(t, actualResult) @@ -184,7 +182,7 @@ func TestIntegrationESOutput(t *testing.T) { p := BackendProcessor(&config.Config{MaxEventSize: 100 * 1024}) var actualResult Result - err = p.HandleStream(ctx, nil, reqDecoderMeta, bytes.NewReader(payload), batchProcessor, &actualResult) + err = p.HandleStream(ctx, reqDecoderMeta, bytes.NewReader(payload), 10, batchProcessor, &actualResult) if test.err != nil { assert.Equal(t, test.err, err) } else { @@ -219,7 +217,7 @@ func TestIntegrationRum(t *testing.T) { p := RUMV2Processor(&config.Config{MaxEventSize: 100 * 1024}) var actualResult Result - err = p.HandleStream(ctx, nil, &reqDecoderMeta, bytes.NewReader(payload), batchProcessor, &actualResult) + err = p.HandleStream(ctx, &reqDecoderMeta, bytes.NewReader(payload), 10, batchProcessor, &actualResult) require.NoError(t, err) assert.Equal(t, Result{Accepted: accepted}, actualResult) }) @@ -250,106 +248,13 @@ func TestRUMV3(t *testing.T) { p := RUMV3Processor(&config.Config{MaxEventSize: 100 * 1024}) var actualResult Result - err = p.HandleStream(ctx, nil, &reqDecoderMeta, bytes.NewReader(payload), batchProcessor, &actualResult) + err = p.HandleStream(ctx, &reqDecoderMeta, bytes.NewReader(payload), 10, batchProcessor, &actualResult) require.NoError(t, err) assert.Equal(t, Result{Accepted: accepted}, actualResult) }) } } -func TestRUMAllowedServiceNames(t *testing.T) { - payload, err := ioutil.ReadFile("../../testdata/intake-v2/transactions_spans_rum.ndjson") - require.NoError(t, err) - - for _, test := range []struct { - AllowServiceNames []string - Allowed bool - }{{ - AllowServiceNames: nil, - Allowed: true, // none specified = all allowed - }, { - AllowServiceNames: []string{"apm-agent-js"}, // matches what's in test data - Allowed: true, - }, { - AllowServiceNames: []string{"reject_everything"}, - Allowed: false, - }} { - p := RUMV2Processor(&config.Config{ - MaxEventSize: 100 * 1024, - RumConfig: config.RumConfig{AllowServiceNames: test.AllowServiceNames}, - }) - - var result Result - err := p.HandleStream( - context.Background(), nil, &model.Metadata{}, bytes.NewReader(payload), - modelprocessor.Nop{}, &result, - ) - if test.Allowed { - require.NoError(t, err) - assert.Equal(t, Result{Accepted: 2}, result) - } else { - assert.EqualError(t, err, "service name is not allowed") - assert.Equal(t, Result{Accepted: 0}, result) - } - } -} - -func TestRateLimiting(t *testing.T) { - payload, err := ioutil.ReadFile("../../testdata/intake-v2/ratelimit.ndjson") - require.NoError(t, err) - - for _, test := range []struct { - name string - lim *rate.Limiter - hit int - accepted int - limited bool - }{{ - name: "LimiterDenyAll", - lim: rate.NewLimiter(rate.Limit(0), 2), - accepted: 0, - limited: true, - }, { - name: "LimiterAllowAll", - lim: rate.NewLimiter(rate.Limit(40), 40*5), - accepted: 19, - }, { - name: "LimiterPartiallyUsedLimitAllow", - lim: rate.NewLimiter(rate.Limit(10), 10*2), - hit: 10, - accepted: 19, - }, { - name: "LimiterPartiallyUsedLimitDeny", - lim: rate.NewLimiter(rate.Limit(7), 7*2), - hit: 10, - accepted: 10, - limited: true, - }, { - name: "LimiterDeny", - lim: rate.NewLimiter(rate.Limit(6), 6*2), - accepted: 10, - limited: true, - }} { - t.Run(test.name, func(t *testing.T) { - if test.hit > 0 { - assert.True(t, test.lim.AllowN(time.Now(), test.hit)) - } - - var actualResult Result - err := BackendProcessor(&config.Config{MaxEventSize: 100 * 1024}).HandleStream( - context.Background(), test.lim, &model.Metadata{}, bytes.NewReader(payload), nopBatchProcessor{}, - &actualResult, - ) - if test.limited { - assert.Equal(t, ErrRateLimitExceeded, err) - } else { - assert.NoError(t, err) - } - assert.Equal(t, Result{Accepted: test.accepted}, actualResult) - }) - } -} - func makeApproveEventsBatchProcessor(t *testing.T, name string, count *int) model.BatchProcessor { return model.ProcessBatchFunc(func(ctx context.Context, b *model.Batch) error { events := b.Transform(ctx, &transform.Config{DataStreams: true}) From 295d1c1acea23e57507af45906a96a2e41108244 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Mon, 21 Jun 2021 14:58:20 +0800 Subject: [PATCH 44/55] docker-compose: fleet-server depends on kibana (#5496) --- docker-compose.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/docker-compose.yml b/docker-compose.yml index 23a6e341422..5cb435d8ad1 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -106,6 +106,7 @@ services: KIBANA_PASSWORD: "${ES_SUPERUSER_PASS:-changeme}" depends_on: elasticsearch: { condition: service_healthy } + kibana: { condition: service_healthy } volumes: - "./testing/docker/fleet-server/certificate.pem:/etc/pki/tls/certs/fleet-server.pem" - "./testing/docker/fleet-server/key.pem:/etc/pki/tls/private/fleet-server-key.pem" From 477822c6a3f67d3545be2f1ccfa79d0058c6c3be Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Mon, 21 Jun 2021 21:29:27 +0800 Subject: [PATCH 45/55] beater: even more refactoring (#5502) * beater: even more refactoring - rate limiting middleware is now installed for both RUM and backend agent APIs, but only applies for anonymous clients (currently only RUM) - rate limiting middleware now performs an initial Allow check at the request level, for consistent request rate limiting of those endpoints that are rate limited - agent config now restricts "insecure" (RUM) agents on the basis that they are anonymous, rather than being RUM specifically. The list of insecure agent names (those allowed for anonymous auth) is now passed in * make gofmt * beater/api/profile: remove unused field --- beater/api/config/agent/handler.go | 63 +++++------ beater/api/config/agent/handler_test.go | 89 +++++++-------- beater/api/intake/handler.go | 33 ++---- beater/api/intake/handler_test.go | 14 ++- beater/api/mux.go | 86 +++++++++----- beater/api/mux_intake_rum_test.go | 4 +- beater/api/mux_test.go | 7 +- beater/api/profile/handler.go | 20 ++-- beater/api/profile/handler_test.go | 17 +-- .../ratelimit/context.go} | 30 +++-- beater/api/ratelimit/store.go | 17 +-- beater/api/ratelimit/store_test.go | 29 +---- beater/http.go | 14 ++- beater/middleware/authorization_middleware.go | 1 + beater/middleware/rate_limit_middleware.go | 31 +++-- .../middleware/rate_limit_middleware_test.go | 107 ++++++++++++++++++ .../middleware/request_metadata_middleware.go | 46 -------- .../request_metadata_middleware_test.go | 77 ------------- beater/request/context.go | 36 ++---- beater/request/context_test.go | 11 +- beater/server.go | 28 +++-- beater/tracing.go | 14 ++- systemtest/rum_test.go | 7 +- 23 files changed, 403 insertions(+), 378 deletions(-) rename beater/{middleware/rum_middleware.go => api/ratelimit/context.go} (52%) create mode 100644 beater/middleware/rate_limit_middleware_test.go delete mode 100644 beater/middleware/request_metadata_middleware.go delete mode 100644 beater/middleware/request_metadata_middleware_test.go diff --git a/beater/api/config/agent/handler.go b/beater/api/config/agent/handler.go index 04086dcc9d2..f77a39f24fa 100644 --- a/beater/api/config/agent/handler.go +++ b/beater/api/config/agent/handler.go @@ -52,18 +52,21 @@ var ( registry = monitoring.Default.NewRegistry("apm-server.acm") errCacheControl = fmt.Sprintf("max-age=%v, must-revalidate", errMaxAgeDuration.Seconds()) - - // rumAgents keywords (new and old) - rumAgents = []string{"rum-js", "js-base"} ) type handler struct { f agentcfg.Fetcher + allowAnonymousAgents []string cacheControl, defaultServiceEnvironment string } -func NewHandler(f agentcfg.Fetcher, config config.KibanaAgentConfig, defaultServiceEnvironment string) request.Handler { +func NewHandler( + f agentcfg.Fetcher, + config config.KibanaAgentConfig, + defaultServiceEnvironment string, + allowAnonymousAgents []string, +) request.Handler { if f == nil { panic("fetcher must not be nil") } @@ -72,6 +75,7 @@ func NewHandler(f agentcfg.Fetcher, config config.KibanaAgentConfig, defaultServ f: f, cacheControl: cacheControl, defaultServiceEnvironment: defaultServiceEnvironment, + allowAnonymousAgents: allowAnonymousAgents, } return h.Handle @@ -83,13 +87,6 @@ func (h *handler) Handle(c *request.Context) { // error handling c.Header().Set(headers.CacheControl, errCacheControl) - ok := c.RateLimiter == nil || c.RateLimiter.Allow() - if !ok { - c.Result.SetDefault(request.IDResponseErrorsRateLimit) - c.Write() - return - } - query, queryErr := buildQuery(c) if queryErr != nil { extractQueryError(c, queryErr) @@ -100,26 +97,29 @@ func (h *handler) Handle(c *request.Context) { query.Service.Environment = h.defaultServiceEnvironment } - if !c.AuthResult.Anonymous { - // The exact agent is not always known for anonymous clients, so we do not - // issue a secondary authorization check for them. Instead, we issue the - // request and filter the results using query.InsecureAgents. - authResource := authorization.Resource{ServiceName: query.Service.Name} - if result, err := authorization.AuthorizedFor(c.Request.Context(), authResource); err != nil { - c.Result.SetDefault(request.IDResponseErrorsServiceUnavailable) - c.Result.Err = err - c.Write() - return - } else if !result.Authorized { - id := request.IDResponseErrorsUnauthorized - status := request.MapResultIDToStatus[id] - if result.Reason != "" { - status.Keyword = result.Reason - } - c.Result.Set(id, status.Code, status.Keyword, nil, nil) - c.Write() - return + // Only service, and not agent, is known for config queries. + // For anonymous/untrusted agents, we filter the results using + // query.InsecureAgents below. + authResource := authorization.Resource{ServiceName: query.Service.Name} + authResult, err := authorization.AuthorizedFor(c.Request.Context(), authResource) + if err != nil { + c.Result.SetDefault(request.IDResponseErrorsServiceUnavailable) + c.Result.Err = err + c.Write() + return + } + if !authResult.Authorized { + id := request.IDResponseErrorsUnauthorized + status := request.MapResultIDToStatus[id] + if authResult.Reason != "" { + status.Keyword = authResult.Reason } + c.Result.Set(id, status.Code, status.Keyword, nil, nil) + c.Write() + return + } + if authResult.Anonymous { + query.InsecureAgents = h.allowAnonymousAgents } result, err := h.f.Fetch(c.Request.Context(), query) @@ -184,9 +184,6 @@ func buildQuery(c *request.Context) (agentcfg.Query, error) { return query, errors.New(agentcfg.ServiceName + " is required") } - if c.IsRum { - query.InsecureAgents = rumAgents - } query.Etag = ifNoneMatch(c) return query, nil } diff --git a/beater/api/config/agent/handler_test.go b/beater/api/config/agent/handler_test.go index 7dcbc1dc079..7ab065f84d3 100644 --- a/beater/api/config/agent/handler_test.go +++ b/beater/api/config/agent/handler_test.go @@ -32,7 +32,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.elastic.co/apm/apmtest" - "golang.org/x/time/rate" "github.com/elastic/beats/v7/libbeat/common" libkibana "github.com/elastic/beats/v7/libbeat/kibana" @@ -173,18 +172,12 @@ func TestAgentConfigHandler(t *testing.T) { var cfg = config.KibanaAgentConfig{Cache: config.Cache{Expiration: 4 * time.Second}} for _, tc := range testcases { f := agentcfg.NewKibanaFetcher(tc.kbClient, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "") + h := NewHandler(f, cfg, "", nil) r := httptest.NewRequest(tc.method, target(tc.queryParams), nil) for k, v := range tc.requestHeader { r.Header.Set(k, v) } ctx, w := newRequestContext(r) - ctx.AuthResult.Authorized = true - ctx.Request = withAuthorization(ctx.Request, - authorizedForFunc(func(context.Context, authorization.Resource) (authorization.Result, error) { - return authorization.Result{Authorized: true}, nil - }), - ) h(ctx) require.Equal(t, tc.respStatus, w.Code) @@ -202,32 +195,46 @@ func TestAgentConfigHandlerAnonymousAccess(t *testing.T) { kbClient := kibanatest.MockKibana(http.StatusUnauthorized, m{"error": "Unauthorized"}, mockVersion, true) cfg := config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(kbClient, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "") + h := NewHandler(f, cfg, "", nil) for _, tc := range []struct { - anonymous bool - response string + anonymous bool + response string + authResource *authorization.Resource }{{ - anonymous: false, - response: `{"error":"APM Server is not authorized to query Kibana. Please configure apm-server.kibana.username and apm-server.kibana.password, and ensure the user has the necessary privileges."}`, + anonymous: false, + response: `{"error":"APM Server is not authorized to query Kibana. Please configure apm-server.kibana.username and apm-server.kibana.password, and ensure the user has the necessary privileges."}`, + authResource: &authorization.Resource{ServiceName: "opbeans"}, }, { - anonymous: true, - response: `{"error":"Unauthorized"}`, + anonymous: true, + response: `{"error":"Unauthorized"}`, + authResource: &authorization.Resource{ServiceName: "opbeans"}, }} { r := httptest.NewRequest(http.MethodGet, target(map[string]string{"service.name": "opbeans"}), nil) - ctx, w := newRequestContext(r) - ctx.AuthResult.Authorized = true - ctx.AuthResult.Anonymous = tc.anonymous - ctx.Request = withAuthorization(ctx.Request, authorization.AnonymousAuth{}) - h(ctx) + c, w := newRequestContext(r) + c.AuthResult.Authorized = true + c.AuthResult.Anonymous = tc.anonymous + + var requestedResource *authorization.Resource + c.Request = withAuthorization(c.Request, + authorizedForFunc(func(ctx context.Context, resource authorization.Resource) (authorization.Result, error) { + if requestedResource != nil { + panic("expected only one AuthorizedFor request") + } + requestedResource = &resource + return c.AuthResult, nil + }), + ) + h(c) assert.Equal(t, tc.response+"\n", w.Body.String()) + assert.Equal(t, tc.authResource, requestedResource) } } func TestAgentConfigHandlerAuthorizedForService(t *testing.T) { cfg := config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(nil, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "") + h := NewHandler(f, cfg, "", nil) r := httptest.NewRequest(http.MethodGet, target(map[string]string{"service.name": "opbeans"}), nil) ctx, w := newRequestContext(r) @@ -249,7 +256,7 @@ func TestAgentConfigHandlerAuthorizedForService(t *testing.T) { func TestAgentConfigHandler_NoKibanaClient(t *testing.T) { cfg := config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(nil, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "") + h := NewHandler(f, cfg, "", nil) w := sendRequest(h, httptest.NewRequest(http.MethodPost, "/config", convert.ToReader(m{ "service": m{"name": "opbeans-node"}}))) @@ -268,7 +275,7 @@ func TestAgentConfigHandler_PostOk(t *testing.T) { var cfg = config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(kb, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "") + h := NewHandler(f, cfg, "", nil) w := sendRequest(h, httptest.NewRequest(http.MethodPost, "/config", convert.ToReader(m{ "service": m{"name": "opbeans-node"}}))) @@ -289,7 +296,7 @@ func TestAgentConfigHandler_DefaultServiceEnvironment(t *testing.T) { var cfg = config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(kb, cfg.Cache.Expiration) - h := NewHandler(f, cfg, "default") + h := NewHandler(f, cfg, "default", nil) sendRequest(h, httptest.NewRequest(http.MethodPost, "/config", convert.ToReader(m{"service": m{"name": "opbeans-node", "environment": "specified"}}))) sendRequest(h, httptest.NewRequest(http.MethodPost, "/config", convert.ToReader(m{"service": m{"name": "opbeans-node"}}))) @@ -306,8 +313,6 @@ func TestAgentConfigRum(t *testing.T) { r := httptest.NewRequest(http.MethodPost, "/rum", convert.ToReader(m{ "service": m{"name": "opbeans"}})) ctx, w := newRequestContext(r) - ctx.IsRum = true - ctx.AuthResult.Anonymous = true h(ctx) var actual map[string]string json.Unmarshal(w.Body.Bytes(), &actual) @@ -320,8 +325,6 @@ func TestAgentConfigRumEtag(t *testing.T) { h := getHandler("rum-js") r := httptest.NewRequest(http.MethodGet, "/rum?ifnonematch=123&service.name=opbeans", nil) ctx, w := newRequestContext(r) - ctx.IsRum = true - ctx.AuthResult.Anonymous = true h(ctx) assert.Equal(t, http.StatusNotModified, w.Code, w.Body.String()) } @@ -333,7 +336,7 @@ func TestAgentConfigNotRum(t *testing.T) { ctx, w := newRequestContext(r) ctx.Request = withAuthorization(ctx.Request, authorizedForFunc(func(context.Context, authorization.Resource) (authorization.Result, error) { - return authorization.Result{Authorized: true}, nil + return authorization.Result{Authorized: true, Anonymous: false}, nil }), ) h(ctx) @@ -348,8 +351,6 @@ func TestAgentConfigNoLeak(t *testing.T) { r := httptest.NewRequest(http.MethodPost, "/rum", convert.ToReader(m{ "service": m{"name": "opbeans"}})) ctx, w := newRequestContext(r) - ctx.IsRum = true - ctx.AuthResult.Anonymous = true h(ctx) var actual map[string]string json.Unmarshal(w.Body.Bytes(), &actual) @@ -357,21 +358,6 @@ func TestAgentConfigNoLeak(t *testing.T) { assert.Equal(t, map[string]string{}, actual) } -func TestAgentConfigRateLimit(t *testing.T) { - h := getHandler("rum-js") - r := httptest.NewRequest(http.MethodPost, "/rum", convert.ToReader(m{ - "service": m{"name": "opbeans"}})) - ctx, w := newRequestContext(r) - ctx.IsRum = true - ctx.RateLimiter = rate.NewLimiter(rate.Limit(0), 0) - ctx.AuthResult.Anonymous = true - h(ctx) - var actual map[string]string - json.Unmarshal(w.Body.Bytes(), &actual) - assert.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) - assert.Equal(t, map[string]string{"error": "too many requests"}, actual) -} - func getHandler(agent string) request.Handler { kb := kibanatest.MockKibana(http.StatusOK, m{ "_id": "1", @@ -386,7 +372,7 @@ func getHandler(agent string) request.Handler { }, mockVersion, true) cfg := config.KibanaAgentConfig{Cache: config.Cache{Expiration: time.Nanosecond}} f := agentcfg.NewKibanaFetcher(kb, cfg.Cache.Expiration) - return NewHandler(f, cfg, "") + return NewHandler(f, cfg, "", []string{"rum-js"}) } func TestIfNoneMatch(t *testing.T) { @@ -412,7 +398,7 @@ func TestAgentConfigTraceContext(t *testing.T) { client := kibana.NewConnectingClient(&kibanaCfg) cfg := config.KibanaAgentConfig{Cache: config.Cache{Expiration: 5 * time.Minute}} f := agentcfg.NewKibanaFetcher(client, cfg.Cache.Expiration) - handler := NewHandler(f, cfg, "default") + handler := NewHandler(f, cfg, "default", nil) _, spans, _ := apmtest.WithTransaction(func(ctx context.Context) { // When the handler is called with a context containing // a transaction, the underlying Kibana query should create a span @@ -439,6 +425,7 @@ func newRequestContext(r *http.Request) (*request.Context, *httptest.ResponseRec w := httptest.NewRecorder() ctx := request.NewContext() ctx.Reset(w, r) + ctx.Request = withAnonymousAuthorization(ctx.Request) return ctx, w } @@ -471,6 +458,12 @@ func (c *recordingKibanaClient) Send(ctx context.Context, method string, path st return c.Client.Send(ctx, method, path, params, header, body) } +func withAnonymousAuthorization(req *http.Request) *http.Request { + return withAuthorization(req, authorizedForFunc(func(context.Context, authorization.Resource) (authorization.Result, error) { + return authorization.Result{Authorized: true, Anonymous: true}, nil + })) +} + func withAuthorization(req *http.Request, auth authorization.Authorization) *http.Request { return req.WithContext(authorization.ContextWithAuthorization(req.Context(), auth)) } diff --git a/beater/api/intake/handler.go b/beater/api/intake/handler.go index 2bfb47b7037..3d9a4d54937 100644 --- a/beater/api/intake/handler.go +++ b/beater/api/intake/handler.go @@ -30,6 +30,7 @@ import ( "github.com/elastic/beats/v7/libbeat/monitoring" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/headers" "github.com/elastic/apm-server/beater/request" "github.com/elastic/apm-server/decoder" @@ -52,7 +53,6 @@ var ( errMethodNotAllowed = errors.New("only POST requests are supported") errServerShuttingDown = errors.New("server is shutting down") errInvalidContentType = errors.New("invalid content type") - errRateLimitExceeded = errors.New("rate limit exceeded") ) // StreamHandler is an interface for handling an Elastic APM agent ND-JSON event @@ -68,27 +68,23 @@ type StreamHandler interface { ) error } +// RequestMetadataFunc is a function type supplied to Handler for extracting +// metadata from the request. This is used for conditionally injecting the +// source IP address as `client.ip` for RUM. +type RequestMetadataFunc func(*request.Context) model.Metadata + // Handler returns a request.Handler for managing intake requests for backend and rum events. -func Handler(handler StreamHandler, batchProcessor model.BatchProcessor) request.Handler { +func Handler(handler StreamHandler, requestMetadataFunc RequestMetadataFunc, batchProcessor model.BatchProcessor) request.Handler { return func(c *request.Context) { if err := validateRequest(c); err != nil { writeError(c, err) return } - if c.RateLimiter != nil { - // Call Allow once for each stream to avoid burning CPU before we - // begin reading for the first time. This prevents clients from - // repeatedly connecting and sending < batchSize events and - // disconnecting before being rate limited. - if !c.RateLimiter.Allow() { - writeError(c, errRateLimitExceeded) - return - } - + if limiter, ok := ratelimit.FromContext(c.Request.Context()); ok { // Apply rate limiting after reading but before processing any events. batchProcessor = modelprocessor.Chained{ - rateLimitBatchProcessor(c.RateLimiter, batchSize), + rateLimitBatchProcessor(limiter, batchSize), batchProcessor, } } @@ -99,12 +95,7 @@ func Handler(handler StreamHandler, batchProcessor model.BatchProcessor) request return } - metadata := model.Metadata{ - UserAgent: model.UserAgent{Original: c.RequestMetadata.UserAgent}, - Client: model.Client{IP: c.RequestMetadata.ClientIP}, - System: model.System{IP: c.RequestMetadata.SystemIP}, - } - + metadata := requestMetadataFunc(c) var result stream.Result if err := handler.HandleStream( c.Request.Context(), @@ -132,7 +123,7 @@ func rateLimitBatch(ctx context.Context, limiter *rate.Limiter, batchSize int) e ctx, cancel := context.WithTimeout(ctx, rateLimitTimeout) defer cancel() if err := limiter.WaitN(ctx, batchSize); err != nil { - return errRateLimitExceeded + return ratelimit.ErrRateLimitExceeded } return nil } @@ -191,7 +182,7 @@ func writeStreamResult(c *request.Context, sr *stream.Result) { errID = request.IDResponseErrorsMethodNotAllowed case errors.Is(err, errInvalidContentType): errID = request.IDResponseErrorsValidate - case errors.Is(err, errRateLimitExceeded): + case errors.Is(err, ratelimit.ErrRateLimitExceeded): errID = request.IDResponseErrorsRateLimit } } diff --git a/beater/api/intake/handler_test.go b/beater/api/intake/handler_test.go index 5a4b7cf1b4d..ed4074520f4 100644 --- a/beater/api/intake/handler_test.go +++ b/beater/api/intake/handler_test.go @@ -34,6 +34,7 @@ import ( "golang.org/x/time/rate" "github.com/elastic/apm-server/approvaltest" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/headers" "github.com/elastic/apm-server/beater/request" @@ -136,7 +137,7 @@ func TestIntakeHandler(t *testing.T) { tc.setup(t) // call handler - h := Handler(tc.processor, tc.batchProcessor) + h := Handler(tc.processor, emptyRequestMetadata, tc.batchProcessor) h(tc.c) require.Equal(t, string(tc.id), string(tc.c.Result.ID)) @@ -190,12 +191,15 @@ func TestRateLimiting(t *testing.T) { var tc testcaseIntakeHandler tc.path = "ratelimit.ndjson" tc.setup(t) - tc.c.RateLimiter = test.limiter + + tc.c.Request = tc.c.Request.WithContext( + ratelimit.ContextWithLimiter(tc.c.Request.Context(), test.limiter), + ) if test.preconsumed > 0 { test.limiter.AllowN(time.Now(), test.preconsumed) } - h := Handler(tc.processor, tc.batchProcessor) + h := Handler(tc.processor, emptyRequestMetadata, tc.batchProcessor) h(tc.c) if test.expectLimited { @@ -275,3 +279,7 @@ func compressedRequest(t *testing.T, compressionType string, compressPayload boo req.Header.Set(headers.ContentEncoding, compressionType) return req } + +func emptyRequestMetadata(*request.Context) model.Metadata { + return model.Metadata{} +} diff --git a/beater/api/mux.go b/beater/api/mux.go index 55e9c1ee73f..436a96d515c 100644 --- a/beater/api/mux.go +++ b/beater/api/mux.go @@ -31,6 +31,7 @@ import ( "github.com/elastic/apm-server/beater/api/config/agent" "github.com/elastic/apm-server/beater/api/intake" "github.com/elastic/apm-server/beater/api/profile" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/api/root" "github.com/elastic/apm-server/beater/authorization" "github.com/elastic/apm-server/beater/config" @@ -68,6 +69,13 @@ const ( IntakeRUMV3Path = "/intake/v3/rum/events" ) +var ( + // rumAgents holds the current and previous agent names for the + // RUM JavaScript agent. This is used for restricting which config + // is supplied to anonymous agents. + rumAgents = []string{"rum-js", "js-base"} +) + // NewMux registers apm handlers to paths building up the APM Server API. func NewMux( beatInfo beat.Info, @@ -75,6 +83,7 @@ func NewMux( report publish.Reporter, batchProcessor model.BatchProcessor, fetcher agentcfg.Fetcher, + ratelimitStore *ratelimit.Store, ) (*http.ServeMux, error) { pool := request.NewContextPool() mux := http.NewServeMux() @@ -91,6 +100,7 @@ func NewMux( authBuilder: auth, reporter: report, batchProcessor: batchProcessor, + ratelimitStore: ratelimitStore, } type route struct { @@ -140,33 +150,46 @@ type routeBuilder struct { authBuilder *authorization.Builder reporter publish.Reporter batchProcessor model.BatchProcessor + ratelimitStore *ratelimit.Store } func (r *routeBuilder) profileHandler() (request.Handler, error) { - h := profile.Handler(r.batchProcessor) + requestMetadataFunc := emptyRequestMetadata + if r.cfg.AugmentEnabled { + requestMetadataFunc = backendRequestMetadata + } + h := profile.Handler(requestMetadataFunc, r.batchProcessor) authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeEventWrite.Action) - return middleware.Wrap(h, backendMiddleware(r.cfg, authHandler, profile.MonitoringMap)...) + return middleware.Wrap(h, backendMiddleware(r.cfg, authHandler, r.ratelimitStore, profile.MonitoringMap)...) } func (r *routeBuilder) backendIntakeHandler() (request.Handler, error) { - h := intake.Handler(stream.BackendProcessor(r.cfg), r.batchProcessor) + requestMetadataFunc := emptyRequestMetadata + if r.cfg.AugmentEnabled { + requestMetadataFunc = backendRequestMetadata + } + h := intake.Handler(stream.BackendProcessor(r.cfg), requestMetadataFunc, r.batchProcessor) authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeEventWrite.Action) - return middleware.Wrap(h, backendMiddleware(r.cfg, authHandler, intake.MonitoringMap)...) + return middleware.Wrap(h, backendMiddleware(r.cfg, authHandler, r.ratelimitStore, intake.MonitoringMap)...) } func (r *routeBuilder) rumIntakeHandler(newProcessor func(*config.Config) *stream.Processor) func() (request.Handler, error) { + requestMetadataFunc := emptyRequestMetadata + if r.cfg.AugmentEnabled { + requestMetadataFunc = rumRequestMetadata + } return func() (request.Handler, error) { batchProcessor := r.batchProcessor batchProcessor = batchProcessorWithAllowedServiceNames(batchProcessor, r.cfg.RumConfig.AllowServiceNames) - h := intake.Handler(newProcessor(r.cfg), batchProcessor) - return middleware.Wrap(h, rumMiddleware(r.cfg, nil, intake.MonitoringMap)...) + h := intake.Handler(newProcessor(r.cfg), requestMetadataFunc, batchProcessor) + return middleware.Wrap(h, rumMiddleware(r.cfg, nil, r.ratelimitStore, intake.MonitoringMap)...) } } func (r *routeBuilder) sourcemapHandler() (request.Handler, error) { h := sourcemap.Handler(r.reporter) authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeSourcemapWrite.Action) - return middleware.Wrap(h, sourcemapMiddleware(r.cfg, authHandler)...) + return middleware.Wrap(h, sourcemapMiddleware(r.cfg, authHandler, r.ratelimitStore)...) } func (r *routeBuilder) rootHandler() (request.Handler, error) { @@ -177,26 +200,27 @@ func (r *routeBuilder) rootHandler() (request.Handler, error) { func (r *routeBuilder) backendAgentConfigHandler(f agentcfg.Fetcher) func() (request.Handler, error) { return func() (request.Handler, error) { authHandler := r.authBuilder.ForPrivilege(authorization.PrivilegeAgentConfigRead.Action) - return agentConfigHandler(r.cfg, authHandler, backendMiddleware, f) + return agentConfigHandler(r.cfg, authHandler, r.ratelimitStore, backendMiddleware, f) } } func (r *routeBuilder) rumAgentConfigHandler(f agentcfg.Fetcher) func() (request.Handler, error) { return func() (request.Handler, error) { - return agentConfigHandler(r.cfg, nil, rumMiddleware, f) + return agentConfigHandler(r.cfg, nil, r.ratelimitStore, rumMiddleware, f) } } -type middlewareFunc func(*config.Config, *authorization.Handler, map[request.ResultID]*monitoring.Int) []middleware.Middleware +type middlewareFunc func(*config.Config, *authorization.Handler, *ratelimit.Store, map[request.ResultID]*monitoring.Int) []middleware.Middleware func agentConfigHandler( cfg *config.Config, authHandler *authorization.Handler, + ratelimitStore *ratelimit.Store, middlewareFunc middlewareFunc, f agentcfg.Fetcher, ) (request.Handler, error) { - mw := middlewareFunc(cfg, authHandler, agent.MonitoringMap) - h := agent.NewHandler(f, cfg.KibanaAgentConfig, cfg.DefaultServiceEnvironment) + mw := middlewareFunc(cfg, authHandler, ratelimitStore, agent.MonitoringMap) + h := agent.NewHandler(f, cfg.KibanaAgentConfig, cfg.DefaultServiceEnvironment, rumAgents) if !cfg.Kibana.Enabled && cfg.AgentConfigs == nil { msg := "Agent remote configuration is disabled. " + @@ -219,37 +243,30 @@ func apmMiddleware(m map[request.ResultID]*monitoring.Int) []middleware.Middlewa } } -func backendMiddleware(cfg *config.Config, auth *authorization.Handler, m map[request.ResultID]*monitoring.Int) []middleware.Middleware { +func backendMiddleware(cfg *config.Config, auth *authorization.Handler, ratelimitStore *ratelimit.Store, m map[request.ResultID]*monitoring.Int) []middleware.Middleware { backendMiddleware := append(apmMiddleware(m), middleware.ResponseHeadersMiddleware(cfg.ResponseHeaders), middleware.AuthorizationMiddleware(auth, true), + middleware.AnonymousRateLimitMiddleware(ratelimitStore), ) - if cfg.AugmentEnabled { - backendMiddleware = append(backendMiddleware, middleware.SystemMetadataMiddleware()) - } return backendMiddleware } -func rumMiddleware(cfg *config.Config, _ *authorization.Handler, m map[request.ResultID]*monitoring.Int) []middleware.Middleware { +func rumMiddleware(cfg *config.Config, auth *authorization.Handler, ratelimitStore *ratelimit.Store, m map[request.ResultID]*monitoring.Int) []middleware.Middleware { msg := "RUM endpoint is disabled. " + "Configure the `apm-server.rum` section in apm-server.yml to enable ingestion of RUM events. " + "If you are not using the RUM agent, you can safely ignore this error." rumMiddleware := append(apmMiddleware(m), middleware.ResponseHeadersMiddleware(cfg.ResponseHeaders), middleware.ResponseHeadersMiddleware(cfg.RumConfig.ResponseHeaders), - middleware.SetRumFlagMiddleware(), - middleware.SetIPRateLimitMiddleware(cfg.RumConfig.EventRate), middleware.CORSMiddleware(cfg.RumConfig.AllowOrigins, cfg.RumConfig.AllowHeaders), middleware.AnonymousAuthorizationMiddleware(), - middleware.KillSwitchMiddleware(cfg.RumConfig.Enabled, msg), + middleware.AnonymousRateLimitMiddleware(ratelimitStore), ) - if cfg.AugmentEnabled { - rumMiddleware = append(rumMiddleware, middleware.UserMetadataMiddleware()) - } - return rumMiddleware + return append(rumMiddleware, middleware.KillSwitchMiddleware(cfg.RumConfig.Enabled, msg)) } -func sourcemapMiddleware(cfg *config.Config, auth *authorization.Handler) []middleware.Middleware { +func sourcemapMiddleware(cfg *config.Config, auth *authorization.Handler, ratelimitStore *ratelimit.Store) []middleware.Middleware { msg := "Sourcemap upload endpoint is disabled. " + "Configure the `apm-server.rum` section in apm-server.yml to enable sourcemap uploads. " + "If you are not using the RUM agent, you can safely ignore this error." @@ -257,8 +274,8 @@ func sourcemapMiddleware(cfg *config.Config, auth *authorization.Handler) []midd msg = "When APM Server is managed by Fleet, Sourcemaps must be uploaded directly to Elasticsearch." } enabled := cfg.RumConfig.Enabled && cfg.RumConfig.SourceMapping.Enabled && !cfg.DataStreams.Enabled - return append(backendMiddleware(cfg, auth, sourcemap.MonitoringMap), - middleware.KillSwitchMiddleware(enabled, msg)) + backendMiddleware := backendMiddleware(cfg, auth, ratelimitStore, sourcemap.MonitoringMap) + return append(backendMiddleware, middleware.KillSwitchMiddleware(enabled, msg)) } func rootMiddleware(cfg *config.Config, auth *authorization.Handler) []middleware.Middleware { @@ -288,3 +305,18 @@ func batchProcessorWithAllowedServiceNames(p model.BatchProcessor, allowedServic } return modelprocessor.Chained{restrictServiceName, p} } + +func emptyRequestMetadata(c *request.Context) model.Metadata { + return model.Metadata{} +} + +func backendRequestMetadata(c *request.Context) model.Metadata { + return model.Metadata{System: model.System{IP: c.SourceIP}} +} + +func rumRequestMetadata(c *request.Context) model.Metadata { + return model.Metadata{ + Client: model.Client{IP: c.SourceIP}, + UserAgent: model.UserAgent{Original: c.UserAgent}, + } +} diff --git a/beater/api/mux_intake_rum_test.go b/beater/api/mux_intake_rum_test.go index 5bf99c1ee44..2f5f728a5b2 100644 --- a/beater/api/mux_intake_rum_test.go +++ b/beater/api/mux_intake_rum_test.go @@ -29,6 +29,7 @@ import ( "github.com/elastic/apm-server/approvaltest" "github.com/elastic/apm-server/beater/api/intake" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/headers" "github.com/elastic/apm-server/beater/middleware" @@ -36,6 +37,7 @@ import ( ) func TestOPTIONS(t *testing.T) { + ratelimitStore, _ := ratelimit.NewStore(1, 1, 1) requestTaken := make(chan struct{}, 1) done := make(chan struct{}, 1) @@ -46,7 +48,7 @@ func TestOPTIONS(t *testing.T) { requestTaken <- struct{}{} <-done }, - rumMiddleware(cfg, nil, intake.MonitoringMap)...) + rumMiddleware(cfg, nil, ratelimitStore, intake.MonitoringMap)...) // use this to block the single allowed concurrent requests go func() { diff --git a/beater/api/mux_test.go b/beater/api/mux_test.go index 8ce997c1eda..7b6be8969e8 100644 --- a/beater/api/mux_test.go +++ b/beater/api/mux_test.go @@ -28,6 +28,7 @@ import ( "github.com/elastic/apm-server/agentcfg" "github.com/elastic/apm-server/approvaltest" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/beatertest" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/request" @@ -76,7 +77,8 @@ func requestToMuxerWithHeaderAndQueryString( func requestToMuxer(cfg *config.Config, r *http.Request) (*httptest.ResponseRecorder, error) { nopReporter := func(context.Context, publish.PendingReq) error { return nil } nopBatchProcessor := model.ProcessBatchFunc(func(context.Context, *model.Batch) error { return nil }) - mux, err := NewMux(beat.Info{Version: "1.2.3"}, cfg, nopReporter, nopBatchProcessor, agentcfg.NewFetcher(cfg)) + ratelimitStore, _ := ratelimit.NewStore(1000, 1000, 1000) + mux, err := NewMux(beat.Info{Version: "1.2.3"}, cfg, nopReporter, nopBatchProcessor, agentcfg.NewFetcher(cfg), ratelimitStore) if err != nil { return nil, err } @@ -111,7 +113,8 @@ func testMonitoringMiddleware(t *testing.T, urlPath string, monitoringMap map[re func newTestMux(t *testing.T, cfg *config.Config) http.Handler { nopReporter := func(context.Context, publish.PendingReq) error { return nil } nopBatchProcessor := model.ProcessBatchFunc(func(context.Context, *model.Batch) error { return nil }) - mux, err := NewMux(beat.Info{Version: "1.2.3"}, cfg, nopReporter, nopBatchProcessor, agentcfg.NewFetcher(cfg)) + ratelimitStore, _ := ratelimit.NewStore(1000, 1000, 1000) + mux, err := NewMux(beat.Info{Version: "1.2.3"}, cfg, nopReporter, nopBatchProcessor, agentcfg.NewFetcher(cfg), ratelimitStore) require.NoError(t, err) return mux } diff --git a/beater/api/profile/handler.go b/beater/api/profile/handler.go index bf1c10f7617..e1f30da55fd 100644 --- a/beater/api/profile/handler.go +++ b/beater/api/profile/handler.go @@ -55,8 +55,13 @@ const ( profileContentLengthLimit = 10 * 1024 * 1024 ) +// RequestMetadataFunc is a function type supplied to Handler for extracting +// metadata from the request. This is used for conditionally injecting the +// source IP address as `client.ip` for RUM. +type RequestMetadataFunc func(*request.Context) model.Metadata + // Handler returns a request.Handler for managing profile requests. -func Handler(processor model.BatchProcessor) request.Handler { +func Handler(requestMetadataFunc RequestMetadataFunc, processor model.BatchProcessor) request.Handler { handle := func(c *request.Context) (*result, error) { if c.Request.Method != http.MethodPost { return nil, requestError{ @@ -71,14 +76,6 @@ func Handler(processor model.BatchProcessor) request.Handler { } } - ok := c.RateLimiter == nil || c.RateLimiter.Allow() - if !ok { - return nil, requestError{ - id: request.IDResponseErrorsRateLimit, - err: errors.New("rate limit exceeded"), - } - } - var totalLimitRemaining int64 = profileContentLengthLimit var profiles []*pprof_profile.Profile var profileMetadata model.Metadata @@ -104,10 +101,7 @@ func Handler(processor model.BatchProcessor) request.Handler { } r := &decoder.LimitedReader{R: part, N: metadataContentLengthLimit} dec := decoder.NewJSONDecoder(r) - metadata := model.Metadata{ - UserAgent: model.UserAgent{Original: c.RequestMetadata.UserAgent}, - Client: model.Client{IP: c.RequestMetadata.ClientIP}, - System: model.System{IP: c.RequestMetadata.SystemIP}} + metadata := requestMetadataFunc(c) if err := v2.DecodeMetadata(dec, &metadata); err != nil { if r.N < 0 { return nil, requestError{ diff --git a/beater/api/profile/handler_test.go b/beater/api/profile/handler_test.go index 9a388eea4b3..0fa0f80eb52 100644 --- a/beater/api/profile/handler_test.go +++ b/beater/api/profile/handler_test.go @@ -31,7 +31,6 @@ import ( "strings" "testing" - "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/model" "github.com/stretchr/testify/assert" @@ -45,8 +44,6 @@ import ( const pprofContentType = `application/x-protobuf; messageType="perftools.profiles.Profile"` func TestHandler(t *testing.T) { - var rateLimit, err = ratelimit.NewStore(1, 0, 0) - require.NoError(t, err) for name, tc := range map[string]testcaseIntakeHandler{ "MethodNotAllowed": { r: httptest.NewRequest(http.MethodGet, "/", nil), @@ -60,10 +57,6 @@ func TestHandler(t *testing.T) { }(), id: request.IDResponseErrorsValidate, }, - "RateLimitExceeded": { - rateLimit: rateLimit, - id: request.IDResponseErrorsRateLimit, - }, "Closing": { batchProcessor: func(t *testing.T) model.BatchProcessor { return model.ProcessBatchFunc(func(context.Context, *model.Batch) error { @@ -199,10 +192,7 @@ func TestHandler(t *testing.T) { } { t.Run(name, func(t *testing.T) { tc.setup(t) - if tc.rateLimit != nil { - tc.c.RateLimiter = tc.rateLimit.ForIP(&http.Request{}) - } - Handler(tc.batchProcessor(t))(tc.c) + Handler(emptyRequestMetadata, tc.batchProcessor(t))(tc.c) assert.Equal(t, string(tc.id), string(tc.c.Result.ID)) resultStatus := request.MapResultIDToStatus[tc.id] @@ -225,7 +215,6 @@ type testcaseIntakeHandler struct { c *request.Context w *httptest.ResponseRecorder r *http.Request - rateLimit *ratelimit.Store batchProcessor func(t *testing.T) model.BatchProcessor reports int parts []part @@ -299,3 +288,7 @@ func prettyJSON(v interface{}) string { enc.Encode(v) return buf.String() } + +func emptyRequestMetadata(*request.Context) model.Metadata { + return model.Metadata{} +} diff --git a/beater/middleware/rum_middleware.go b/beater/api/ratelimit/context.go similarity index 52% rename from beater/middleware/rum_middleware.go rename to beater/api/ratelimit/context.go index be3f8f33d4d..d17f99597de 100644 --- a/beater/middleware/rum_middleware.go +++ b/beater/api/ratelimit/context.go @@ -15,18 +15,28 @@ // specific language governing permissions and limitations // under the License. -package middleware +package ratelimit import ( - "github.com/elastic/apm-server/beater/request" + "context" + + "github.com/pkg/errors" + "golang.org/x/time/rate" ) -// SetRumFlagMiddleware sets a rum flag in the context -func SetRumFlagMiddleware() Middleware { - return func(h request.Handler) (request.Handler, error) { - return func(c *request.Context) { - c.IsRum = true - h(c) - }, nil - } +// ErrRateLimitExceeded is returned when the rate limit is exceeded. +var ErrRateLimitExceeded = errors.New("rate limit exceeded") + +type rateLimiterKey struct{} + +// FromContext returns a rate.Limiter if one is contained in ctx, +// and a bool indicating whether one was found. +func FromContext(ctx context.Context) (*rate.Limiter, bool) { + limiter, ok := ctx.Value(rateLimiterKey{}).(*rate.Limiter) + return limiter, ok +} + +// ContextWithLimiter returns a copy of parent associated with limiter. +func ContextWithLimiter(parent context.Context, limiter *rate.Limiter) context.Context { + return context.WithValue(parent, rateLimiterKey{}, limiter) } diff --git a/beater/api/ratelimit/store.go b/beater/api/ratelimit/store.go index b9fac6c8cfd..660a3af76cd 100644 --- a/beater/api/ratelimit/store.go +++ b/beater/api/ratelimit/store.go @@ -18,11 +18,9 @@ package ratelimit import ( - "net/http" + "net" "sync" - "github.com/elastic/apm-server/utility" - "github.com/hashicorp/golang-lru/simplelru" "github.com/pkg/errors" "golang.org/x/time/rate" @@ -63,8 +61,9 @@ func NewStore(size, rateLimit, burstFactor int) (*Store, error) { return &store, nil } -// acquire returns a rate.Limiter instance for the given key -func (s *Store) acquire(key string) *rate.Limiter { +// ForIP returns a rate limiter for the given IP. +func (s *Store) ForIP(ip net.IP) *rate.Limiter { + key := ip.String() // lock get and add action for cache to allow proper eviction handling without // race conditions. @@ -83,11 +82,3 @@ func (s *Store) acquire(key string) *rate.Limiter { } return limiter } - -// ForIP returns a rate limiter for the given request IP -func (s *Store) ForIP(r *http.Request) *rate.Limiter { - if s == nil { - return nil - } - return s.acquire(utility.RemoteAddr(r)) -} diff --git a/beater/api/ratelimit/store_test.go b/beater/api/ratelimit/store_test.go index e6559e089f0..4706e9dbff8 100644 --- a/beater/api/ratelimit/store_test.go +++ b/beater/api/ratelimit/store_test.go @@ -18,7 +18,7 @@ package ratelimit import ( - "net/http" + "net" "testing" "time" @@ -38,7 +38,6 @@ func TestCacheInitFails(t *testing.T) { c, err := NewStore(test.size, test.limit, 3) assert.Error(t, err) assert.Nil(t, c) - assert.Nil(t, c.ForIP(&http.Request{})) } } @@ -50,20 +49,20 @@ func TestCacheEviction(t *testing.T) { require.NoError(t, err) // add new limiter - rlA := store.acquire("a") + rlA := store.ForIP(net.ParseIP("127.0.0.1")) rlA.AllowN(time.Now(), 3) // add new limiter - rlB := store.acquire("b") + rlB := store.ForIP(net.ParseIP("127.0.0.2")) rlB.AllowN(time.Now(), 2) // reuse evicted limiter rlA - rlC := store.acquire("c") + rlC := store.ForIP(net.ParseIP("127.0.0.3")) assert.False(t, rlC.Allow()) assert.Equal(t, rlC, store.evictedLimiter) // reuse evicted limiter rlB - rlD := store.acquire("a") + rlD := store.ForIP(net.ParseIP("127.0.0.1")) assert.True(t, rlD.Allow()) assert.False(t, rlD.Allow()) assert.Equal(t, rlD, store.evictedLimiter) @@ -77,22 +76,6 @@ func TestCacheEviction(t *testing.T) { func TestCacheOk(t *testing.T) { store, err := NewStore(1, 1, 1) require.NoError(t, err) - limiter := store.acquire("a") + limiter := store.ForIP(net.ParseIP("127.0.0.1")) assert.NotNil(t, limiter) } - -func TestRateLimitPerIP(t *testing.T) { - store, err := NewStore(2, 1, 1) - require.NoError(t, err) - - var reqFrom = func(ip string) *http.Request { - r := http.Request{} - r.Header = http.Header{} - r.Header.Set("X-Real-Ip", ip) - return &r - } - assert.True(t, store.ForIP(reqFrom("10.10.10.1")).Allow()) - assert.False(t, store.ForIP(reqFrom("10.10.10.1")).Allow()) - assert.True(t, store.ForIP(reqFrom("10.10.10.2")).Allow()) - assert.False(t, store.ForIP(reqFrom("10.10.10.3")).Allow()) -} diff --git a/beater/http.go b/beater/http.go index 994e4b396ed..d7bcb77bab5 100644 --- a/beater/http.go +++ b/beater/http.go @@ -29,6 +29,7 @@ import ( "github.com/elastic/apm-server/agentcfg" "github.com/elastic/apm-server/beater/api" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/model" "github.com/elastic/apm-server/model/modelprocessor" @@ -47,7 +48,16 @@ type httpServer struct { grpcListener net.Listener } -func newHTTPServer(logger *logp.Logger, info beat.Info, cfg *config.Config, tracer *apm.Tracer, reporter publish.Reporter, batchProcessor model.BatchProcessor, f agentcfg.Fetcher) (*httpServer, error) { +func newHTTPServer( + logger *logp.Logger, + info beat.Info, + cfg *config.Config, + tracer *apm.Tracer, + reporter publish.Reporter, + batchProcessor model.BatchProcessor, + agentcfgFetcher agentcfg.Fetcher, + ratelimitStore *ratelimit.Store, +) (*httpServer, error) { // Add a model processor that checks authorization for the agent and service for each event. batchProcessor = modelprocessor.Chained{ @@ -55,7 +65,7 @@ func newHTTPServer(logger *logp.Logger, info beat.Info, cfg *config.Config, trac batchProcessor, } - mux, err := api.NewMux(info, cfg, reporter, batchProcessor, f) + mux, err := api.NewMux(info, cfg, reporter, batchProcessor, agentcfgFetcher, ratelimitStore) if err != nil { return nil, err } diff --git a/beater/middleware/authorization_middleware.go b/beater/middleware/authorization_middleware.go index 2de8adb25f1..724b87881d4 100644 --- a/beater/middleware/authorization_middleware.go +++ b/beater/middleware/authorization_middleware.go @@ -65,6 +65,7 @@ func AnonymousAuthorizationMiddleware() Middleware { return func(h request.Handler) (request.Handler, error) { return func(c *request.Context) { auth := authorization.AnonymousAuth{} + c.AuthResult = authorization.Result{Authorized: true, Anonymous: true} c.Request = c.Request.WithContext(authorization.ContextWithAuthorization(c.Request.Context(), auth)) h(c) }, nil diff --git a/beater/middleware/rate_limit_middleware.go b/beater/middleware/rate_limit_middleware.go index 8b71f65cfe6..cb954432517 100644 --- a/beater/middleware/rate_limit_middleware.go +++ b/beater/middleware/rate_limit_middleware.go @@ -19,20 +19,33 @@ package middleware import ( "github.com/elastic/apm-server/beater/api/ratelimit" - "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/request" ) -const burstMultiplier = 3 - -// SetIPRateLimitMiddleware sets a rate limiter -func SetIPRateLimitMiddleware(cfg config.EventRate) Middleware { - store, err := ratelimit.NewStore(cfg.LruSize, cfg.Limit, burstMultiplier) - +// AnonymousRateLimitMiddleware adds a rate.Limiter to the context of anonymous +// requests, first ensuring the client is allowed to perform a single event and +// responding with 429 Too Many Requests if it is not. +// +// This middleware must be wrapped by AuthorizationMiddleware, as it depends on +// the value of c.AuthResult.Anonymous. +func AnonymousRateLimitMiddleware(store *ratelimit.Store) Middleware { return func(h request.Handler) (request.Handler, error) { return func(c *request.Context) { - c.RateLimiter = store.ForIP(c.Request) + if c.AuthResult.Anonymous { + limiter := store.ForIP(c.SourceIP) + if !limiter.Allow() { + c.Result.SetWithError( + request.IDResponseErrorsRateLimit, + ratelimit.ErrRateLimitExceeded, + ) + c.Write() + return + } + ctx := c.Request.Context() + ctx = ratelimit.ContextWithLimiter(ctx, limiter) + c.Request = c.Request.WithContext(ctx) + } h(c) - }, err + }, nil } } diff --git a/beater/middleware/rate_limit_middleware_test.go b/beater/middleware/rate_limit_middleware_test.go new file mode 100644 index 00000000000..bd6c9d18b89 --- /dev/null +++ b/beater/middleware/rate_limit_middleware_test.go @@ -0,0 +1,107 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/elastic/apm-server/beater/api/ratelimit" + "github.com/elastic/apm-server/beater/request" +) + +func TestAnonymousRateLimitMiddleware(t *testing.T) { + type test struct { + burst int + anonymous bool + + expectStatusCode int + expectAllow bool + } + for _, test := range []test{{ + burst: 0, + anonymous: false, + expectStatusCode: http.StatusOK, + }, { + burst: 0, + anonymous: true, + expectStatusCode: http.StatusTooManyRequests, + }, { + burst: 1, + anonymous: true, + expectStatusCode: http.StatusOK, + expectAllow: false, + }, { + burst: 2, + anonymous: true, + expectStatusCode: http.StatusOK, + expectAllow: true, + }} { + store, _ := ratelimit.NewStore(1, 1, test.burst) + middleware := AnonymousRateLimitMiddleware(store) + handler := func(c *request.Context) { + limiter, ok := ratelimit.FromContext(c.Request.Context()) + if test.anonymous { + require.True(t, ok) + assert.Equal(t, test.expectAllow, limiter.Allow()) + } else { + require.False(t, ok) + } + } + wrapped, err := middleware(handler) + require.NoError(t, err) + + c := request.NewContext() + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + c.Reset(w, r) + c.AuthResult.Anonymous = test.anonymous + + wrapped(c) + assert.Equal(t, test.expectStatusCode, w.Code) + } +} + +func TestAnonymousRateLimitMiddlewareForIP(t *testing.T) { + store, _ := ratelimit.NewStore(2, 1, 1) + middleware := AnonymousRateLimitMiddleware(store) + handler := func(c *request.Context) {} + wrapped, err := middleware(handler) + require.NoError(t, err) + + requestWithIP := func(ip string) int { + c := request.NewContext() + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = ip + c.Reset(w, r) + c.AuthResult.Anonymous = true + wrapped(c) + return w.Code + } + assert.Equal(t, http.StatusOK, requestWithIP("10.1.1.1")) + assert.Equal(t, http.StatusTooManyRequests, requestWithIP("10.1.1.1")) + assert.Equal(t, http.StatusOK, requestWithIP("10.1.1.2")) + + // ratelimit.Store size is 2: the 3rd IP reuses an existing (depleted) rate limiter. + assert.Equal(t, http.StatusTooManyRequests, requestWithIP("10.1.1.3")) +} diff --git a/beater/middleware/request_metadata_middleware.go b/beater/middleware/request_metadata_middleware.go deleted file mode 100644 index 9bb57d44808..00000000000 --- a/beater/middleware/request_metadata_middleware.go +++ /dev/null @@ -1,46 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package middleware - -import ( - "github.com/elastic/apm-server/beater/request" - "github.com/elastic/apm-server/utility" -) - -// UserMetadataMiddleware returns a Middleware recording request-level -// user metadata (e.g. user-agent and source IP) in the request's context. -func UserMetadataMiddleware() Middleware { - return func(h request.Handler) (request.Handler, error) { - return func(c *request.Context) { - c.RequestMetadata.UserAgent = utility.UserAgentHeader(c.Request.Header) - c.RequestMetadata.ClientIP = utility.ExtractIP(c.Request) - h(c) - }, nil - } -} - -// SystemMetadataMiddleware returns a Middleware recording request-level -// system metadata (e.g. source IP) in the request's context. -func SystemMetadataMiddleware() Middleware { - return func(h request.Handler) (request.Handler, error) { - return func(c *request.Context) { - c.RequestMetadata.SystemIP = utility.ExtractIP(c.Request) - h(c) - }, nil - } -} diff --git a/beater/middleware/request_metadata_middleware_test.go b/beater/middleware/request_metadata_middleware_test.go deleted file mode 100644 index ac69123c5c3..00000000000 --- a/beater/middleware/request_metadata_middleware_test.go +++ /dev/null @@ -1,77 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package middleware - -import ( - "fmt" - "net" - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/elastic/apm-server/beater/beatertest" -) - -func TestUserMetadataMiddleware(t *testing.T) { - type test struct { - remoteAddr string - userAgent []string - expectedIP net.IP - expectedUserAgent string - } - - ua1 := "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36" - ua2 := "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:67.0) Gecko/20100101 Firefox/67.0" - tests := []test{ - {remoteAddr: "1.2.3.4:1234", expectedIP: net.ParseIP("1.2.3.4"), userAgent: []string{ua1, ua2}, expectedUserAgent: fmt.Sprintf("%s, %s", ua1, ua2)}, - {remoteAddr: "not-an-ip:1234", userAgent: []string{ua1}, expectedUserAgent: ua1}, - {remoteAddr: ""}, - } - - for _, test := range tests { - c, _ := beatertest.DefaultContextWithResponseRecorder() - c.Request.RemoteAddr = test.remoteAddr - for _, ua := range test.userAgent { - c.Request.Header.Add("User-Agent", ua) - } - - Apply(UserMetadataMiddleware(), beatertest.HandlerIdle)(c) - assert.Equal(t, test.expectedUserAgent, c.RequestMetadata.UserAgent) - assert.Equal(t, test.expectedIP, c.RequestMetadata.ClientIP) - } -} - -func TestSystemMetadataMiddleware(t *testing.T) { - type test struct { - remoteAddr string - expectedIP net.IP - } - tests := []test{ - {remoteAddr: "1.2.3.4:1234", expectedIP: net.ParseIP("1.2.3.4")}, - {remoteAddr: "not-an-ip:1234"}, - {remoteAddr: ""}, - } - - for _, test := range tests { - c, _ := beatertest.DefaultContextWithResponseRecorder() - c.Request.RemoteAddr = test.remoteAddr - - Apply(SystemMetadataMiddleware(), beatertest.HandlerIdle)(c) - assert.Equal(t, test.expectedIP, c.RequestMetadata.SystemIP) - } -} diff --git a/beater/request/context.go b/beater/request/context.go index 118643ef412..4478ce9a8e3 100644 --- a/beater/request/context.go +++ b/beater/request/context.go @@ -23,13 +23,12 @@ import ( "net/http" "strings" - "golang.org/x/time/rate" - "github.com/elastic/beats/v7/libbeat/logp" "github.com/elastic/apm-server/beater/authorization" "github.com/elastic/apm-server/beater/headers" logs "github.com/elastic/apm-server/log" + "github.com/elastic/apm-server/utility" ) const ( @@ -43,26 +42,17 @@ var ( // Context abstracts request and response information for http requests type Context struct { - Request *http.Request - Logger *logp.Logger - RateLimiter *rate.Limiter - AuthResult authorization.Result - IsRum bool - Result Result - RequestMetadata Metadata + Request *http.Request + Logger *logp.Logger + AuthResult authorization.Result + Result Result + SourceIP net.IP + UserAgent string w http.ResponseWriter writeAttempts int } -// Metadata contains metadata extracted from the request by middleware, -// and should be merged into the event metadata. -type Metadata struct { - ClientIP net.IP - SystemIP net.IP - UserAgent string -} - // NewContext creates an empty Context struct func NewContext() *Context { return &Context{} @@ -72,23 +62,15 @@ func NewContext() *Context { func (c *Context) Reset(w http.ResponseWriter, r *http.Request) { c.Request = r c.Logger = nil - c.RateLimiter = nil c.AuthResult = authorization.Result{} - c.IsRum = false c.Result.Reset() - c.RequestMetadata.Reset() + c.SourceIP = utility.ExtractIP(r) + c.UserAgent = utility.UserAgentHeader(r.Header) c.w = w c.writeAttempts = 0 } -// Reset sets all attribtues of the Metadata instance to it's zero value -func (m *Metadata) Reset() { - m.ClientIP = nil - m.SystemIP = nil - m.UserAgent = "" -} - // Header returns the http.Header of the context's writer func (c *Context) Header() http.Header { return c.w.Header() diff --git a/beater/request/context_test.go b/beater/request/context_test.go index aa916ac5158..2ec5eb9e1ed 100644 --- a/beater/request/context_test.go +++ b/beater/request/context_test.go @@ -18,6 +18,7 @@ package request import ( + "net" "net/http" "net/http/httptest" "reflect" @@ -37,7 +38,11 @@ func TestContext_Reset(t *testing.T) { w1.WriteHeader(http.StatusServiceUnavailable) w2 := httptest.NewRecorder() r1 := httptest.NewRequest(http.MethodGet, "/", nil) + r1.RemoteAddr = "10.1.2.3:4321" + r1.Header.Set("User-Agent", "ua1") r2 := httptest.NewRequest(http.MethodHead, "/new", nil) + r2.RemoteAddr = "10.1.2.3:1234" + r2.Header.Set("User-Agent", "ua2") c := Context{ Request: r1, w: w1, @@ -65,8 +70,10 @@ func TestContext_Reset(t *testing.T) { assert.Equal(t, 0, c.writeAttempts) case "Result": assertResultIsEmpty(t, cVal.Field(i).Interface().(Result)) - case "RequestMetadata": - assert.Equal(t, Metadata{}, cVal.Field(i).Interface().(Metadata)) + case "SourceIP": + assert.Equal(t, net.ParseIP("10.1.2.3"), cVal.Field(i).Interface()) + case "UserAgent": + assert.Equal(t, "ua2", cVal.Field(i).Interface()) default: assert.Empty(t, cVal.Field(i).Interface(), cType.Field(i).Name) } diff --git a/beater/server.go b/beater/server.go index 7d144321402..11179ebb31b 100644 --- a/beater/server.go +++ b/beater/server.go @@ -32,6 +32,7 @@ import ( "github.com/elastic/beats/v7/libbeat/version" "github.com/elastic/apm-server/agentcfg" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/authorization" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/beater/interceptors" @@ -116,16 +117,24 @@ func newServer( reporter publish.Reporter, batchProcessor model.BatchProcessor, ) (server, error) { - fetcher := agentcfg.NewFetcher(cfg) - httpServer, err := newHTTPServer(logger, info, cfg, tracer, reporter, batchProcessor, fetcher) + agentcfgFetcher := agentcfg.NewFetcher(cfg) + ratelimitStore, err := ratelimit.NewStore( + cfg.RumConfig.EventRate.LruSize, + cfg.RumConfig.EventRate.Limit, + 3, // burst multiplier + ) if err != nil { return server{}, err } - grpcServer, err := newGRPCServer(logger, cfg, tracer, batchProcessor, httpServer.TLSConfig, fetcher) + httpServer, err := newHTTPServer(logger, info, cfg, tracer, reporter, batchProcessor, agentcfgFetcher, ratelimitStore) if err != nil { return server{}, err } - jaegerServer, err := jaeger.NewServer(logger, cfg, tracer, batchProcessor, fetcher) + grpcServer, err := newGRPCServer(logger, cfg, tracer, batchProcessor, httpServer.TLSConfig, agentcfgFetcher, ratelimitStore) + if err != nil { + return server{}, err + } + jaegerServer, err := jaeger.NewServer(logger, cfg, tracer, batchProcessor, agentcfgFetcher) if err != nil { return server{}, err } @@ -144,7 +153,8 @@ func newGRPCServer( tracer *apm.Tracer, batchProcessor model.BatchProcessor, tlsConfig *tls.Config, - fetcher agentcfg.Fetcher, + agentcfgFetcher agentcfg.Fetcher, + ratelimitStore *ratelimit.Store, ) (*grpc.Server, error) { // TODO(axw) share auth builder with beater/api. authBuilder, err := authorization.NewBuilder(cfg.AgentAuth) @@ -152,10 +162,11 @@ func newGRPCServer( return nil, err } - // NOTE(axw) even if TLS is enabled we should not use grpc.Creds, as TLS is handled by the net/http server. apmInterceptor := apmgrpc.NewUnaryServerInterceptor(apmgrpc.WithRecovery(), apmgrpc.WithTracer(tracer)) authInterceptor := newAuthUnaryServerInterceptor(authBuilder) + // Note that we intentionally do not use a grpc.Creds ServerOption + // even if TLS is enabled, as TLS is handled by the net/http server. logger = logger.Named("grpc") srv := grpc.NewServer( grpc.ChainUnaryInterceptor( @@ -165,6 +176,9 @@ func newGRPCServer( interceptors.Metrics(logger, otlp.RegistryMonitoringMaps, jaeger.RegistryMonitoringMaps), interceptors.Timeout(), authInterceptor, + + // TODO(axw) add a rate limiting interceptor here once we've + // updated authInterceptor to handle auth for Jaeger requests. ), ) @@ -182,7 +196,7 @@ func newGRPCServer( batchProcessor, } - jaeger.RegisterGRPCServices(srv, authBuilder, jaeger.ElasticAuthTag, logger, batchProcessor, fetcher) + jaeger.RegisterGRPCServices(srv, authBuilder, jaeger.ElasticAuthTag, logger, batchProcessor, agentcfgFetcher) if err := otlp.RegisterGRPCServices(srv, batchProcessor); err != nil { return nil, err } diff --git a/beater/tracing.go b/beater/tracing.go index 8d37f979ea5..6a9f791795b 100644 --- a/beater/tracing.go +++ b/beater/tracing.go @@ -27,6 +27,7 @@ import ( "github.com/elastic/apm-server/agentcfg" "github.com/elastic/apm-server/beater/api" + "github.com/elastic/apm-server/beater/api/ratelimit" "github.com/elastic/apm-server/beater/config" "github.com/elastic/apm-server/model" "github.com/elastic/apm-server/publish" @@ -59,7 +60,18 @@ func newTracerServer(listener net.Listener, logger *logp.Logger) (*tracerServer, } }) cfg := config.DefaultConfig() - mux, err := api.NewMux(beat.Info{}, cfg, nopReporter, processBatch, agentcfg.NewFetcher(cfg)) + ratelimitStore, err := ratelimit.NewStore(1, 1, 1) // unused, arbitrary params + if err != nil { + return nil, err + } + mux, err := api.NewMux( + beat.Info{}, + cfg, + nopReporter, + processBatch, + agentcfg.NewFetcher(cfg), + ratelimitStore, + ) if err != nil { return nil, err } diff --git a/systemtest/rum_test.go b/systemtest/rum_test.go index 7f72a3ea6d2..4d82b0a71ef 100644 --- a/systemtest/rum_test.go +++ b/systemtest/rum_test.go @@ -192,7 +192,12 @@ func TestRUMRateLimit(t *testing.T) { g.Go(func() error { return sendEvents("10.11.12.13", srv.Config.RUM.RateLimit.EventLimit) }) g.Go(func() error { return sendEvents("10.11.12.14", srv.Config.RUM.RateLimit.EventLimit) }) g.Go(func() error { return sendEvents("10.11.12.15", srv.Config.RUM.RateLimit.EventLimit) }) - assert.EqualError(t, g.Wait(), `429 Too Many Requests ({"accepted":0,"errors":[{"message":"rate limit exceeded"}]})`) + err = g.Wait() + require.Error(t, err) + + // The exact error differs, depending on whether rate limiting was applied at the request + // level, or at the event stream level. Either could occur. + assert.Regexp(t, `429 Too Many Requests .*`, err.Error()) } func sendRUMEventsPayload(t *testing.T, srv *apmservertest.Server, payloadFile string) { From c4dc133941bcfef5ae2e4e53f0b0ca0fd47c4c57 Mon Sep 17 00:00:00 2001 From: Julien Mailleret <8582351+jmlrt@users.noreply.github.com> Date: Tue, 22 Jun 2021 03:45:22 +0200 Subject: [PATCH 46/55] Fix UBI source URL (#5506) This commit fix the source URL for UBI image to ensure that it stays consistent with the URL generated in https://artifacts.elastic.co/reports/dependencies/dependencies-current.html --- script/generate_notice.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/generate_notice.py b/script/generate_notice.py index fd7196f19e0..7916479ecb8 100644 --- a/script/generate_notice.py +++ b/script/generate_notice.py @@ -33,7 +33,7 @@ "version": "8", "url": "https://catalog.redhat.com/software/containers/ubi8/ubi-minimal/5c359a62bed8bd75a2c3fba8", "license": "Custom;https://www.redhat.com/licenses/EULA_Red_Hat_Universal_Base_Image_English_20190422.pdf", - "sourceURL": "https://oss-dependencies.elastic.co/redhat/ubi/ubi-minimal-8-source.tar.gz", + "sourceURL": "https://oss-dependencies.elastic.co/red-hat-universal-base-image-minimal/8/ubi-minimal-8-source.tar.gz", }] From 67bb80d8517147d8f0a2fdc322d04510d64e7a27 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 22 Jun 2021 15:22:58 +0800 Subject: [PATCH 47/55] systemtest: remove TestDataStream*, fix Fleet test (#5503) Remove the TestDataStreams tests, which test the server's internal/private/undocumneted configuration for enabling data streams. At least for now, this is only intended to be used with Fleet, so we should instead just rely on the Fleet system test. There was a bug in the Fleet test where apm-server would create "traces-apm-default" as a plain old index instead of a data stream in certain conditions: if the apm integration had previously been installed, then the traces-apm template would be deleted by CleanupElasticsearch. Because the package was already installed, creating the policy would not automatically reinstall it and add the missing template. We fix the Fleet issue by uninstalling the package during Fleet cleanup, after unenrolling agents. --- systemtest/apmservertest/filter.go | 21 ++- systemtest/apmservertest/server.go | 7 +- .../false.approved.json | 76 ---------- ...son => TestFleetIntegration.approved.json} | 4 +- systemtest/datastreams_test.go | 140 ------------------ systemtest/fleet_test.go | 52 ++++++- systemtest/fleettest/client.go | 11 ++ 7 files changed, 76 insertions(+), 235 deletions(-) delete mode 100644 systemtest/approvals/TestDataStreamsEnabled/false.approved.json rename systemtest/approvals/{TestDataStreamsEnabled/true.approved.json => TestFleetIntegration.approved.json} (94%) delete mode 100644 systemtest/datastreams_test.go diff --git a/systemtest/apmservertest/filter.go b/systemtest/apmservertest/filter.go index a851e1fd772..d41926a9394 100644 --- a/systemtest/apmservertest/filter.go +++ b/systemtest/apmservertest/filter.go @@ -29,7 +29,7 @@ import ( "go.elastic.co/fastjson" ) -// TODO(axw) move EventMetadata and filteringTransport to go.elastic.co/apmtest, +// TODO(axw) move EventMetadata and FilteringTransport to go.elastic.co/apmtest, // generalising filteringTransport to work with arbitrary base transports. To do // that we would need to dynamically check for optional interfaces supported by // the base transport, and create passthrough methods. @@ -47,14 +47,22 @@ type EventMetadataFilter interface { FilterEventMetadata(*EventMetadata) } -type filteringTransport struct { +// FilteringTransport is a transport for the APM Go agent which modifies events +// prior to sending them to the underlying transport. +type FilteringTransport struct { *transport.HTTPTransport filter EventMetadataFilter } +// NewFilteringTransport returns a new FilteringTransport that filters events +// using f, and sends them on to h. +func NewFilteringTransport(h *transport.HTTPTransport, f EventMetadataFilter) *FilteringTransport { + return &FilteringTransport{h, f} +} + // SendStream decodes metadata from reader, passes it through the filters, // and then sends the modified stream to the underlying transport. -func (t *filteringTransport) SendStream(ctx context.Context, stream io.Reader) error { +func (t *FilteringTransport) SendStream(ctx context.Context, stream io.Reader) error { zr, err := zlib.NewReader(stream) if err != nil { return err @@ -98,9 +106,12 @@ func (t *filteringTransport) SendStream(ctx context.Context, stream io.Reader) e return t.HTTPTransport.SendStream(ctx, &buf) } -type defaultMetadataFilter struct{} +// DefaultMetadataFilter implements EventMetadataFilter, setting some default values +// for fields that would otherwise by dynamically discovered. +type DefaultMetadataFilter struct{} -func (defaultMetadataFilter) FilterEventMetadata(m *EventMetadata) { +// FilterEventMetadata updates m with default values for dynamically discovered fields. +func (DefaultMetadataFilter) FilterEventMetadata(m *EventMetadata) { m.System.Platform = "minix" m.System.Architecture = "i386" m.System.Container = nil diff --git a/systemtest/apmservertest/server.go b/systemtest/apmservertest/server.go index 63264cb6d33..f180a92f97e 100644 --- a/systemtest/apmservertest/server.go +++ b/systemtest/apmservertest/server.go @@ -117,7 +117,7 @@ func NewServer(tb testing.TB, args ...string) *Server { func NewUnstartedServer(tb testing.TB, args ...string) *Server { return &Server{ Config: DefaultConfig(), - EventMetadataFilter: defaultMetadataFilter{}, + EventMetadataFilter: DefaultMetadataFilter{}, tb: tb, args: args, } @@ -469,10 +469,7 @@ func (s *Server) Tracer() *apm.Tracer { var transport transport.Transport = httpTransport if s.EventMetadataFilter != nil { - transport = &filteringTransport{ - HTTPTransport: httpTransport, - filter: s.EventMetadataFilter, - } + transport = NewFilteringTransport(httpTransport, s.EventMetadataFilter) } tracer, err := apm.NewTracerOptions(apm.TracerOptions{Transport: transport}) if err != nil { diff --git a/systemtest/approvals/TestDataStreamsEnabled/false.approved.json b/systemtest/approvals/TestDataStreamsEnabled/false.approved.json deleted file mode 100644 index 5b4a28ca9c8..00000000000 --- a/systemtest/approvals/TestDataStreamsEnabled/false.approved.json +++ /dev/null @@ -1,76 +0,0 @@ -{ - "events": [ - { - "@timestamp": "dynamic", - "agent": { - "name": "go", - "version": "0.0.0" - }, - "ecs": { - "version": "dynamic" - }, - "event": { - "ingested": "dynamic", - "outcome": "unknown" - }, - "host": { - "architecture": "i386", - "hostname": "beowulf", - "ip": "127.0.0.1", - "name": "beowulf", - "os": { - "platform": "minix" - } - }, - "observer": { - "ephemeral_id": "dynamic", - "hostname": "dynamic", - "id": "dynamic", - "type": "apm-server", - "version": "dynamic", - "version_major": "dynamic" - }, - "process": { - "pid": 1, - "title": "systemtest.test" - }, - "processor": { - "event": "transaction", - "name": "transaction" - }, - "service": { - "language": { - "name": "go", - "version": "2.0" - }, - "name": "systemtest", - "node": { - "name": "beowulf" - }, - "runtime": { - "name": "gc", - "version": "2.0" - } - }, - "timestamp": { - "us": "dynamic" - }, - "trace": { - "id": "dynamic" - }, - "transaction": { - "duration": { - "us": 1000000 - }, - "id": "dynamic", - "name": "name", - "sampled": true, - "span_count": { - "dropped": 0, - "started": 0 - }, - "type": "type" - } - } - ] -} diff --git a/systemtest/approvals/TestDataStreamsEnabled/true.approved.json b/systemtest/approvals/TestFleetIntegration.approved.json similarity index 94% rename from systemtest/approvals/TestDataStreamsEnabled/true.approved.json rename to systemtest/approvals/TestFleetIntegration.approved.json index 28ca86e8ed5..09739f5aa71 100644 --- a/systemtest/approvals/TestDataStreamsEnabled/true.approved.json +++ b/systemtest/approvals/TestFleetIntegration.approved.json @@ -13,12 +13,14 @@ "version": "dynamic" }, "event": { + "agent_id_status": "untrusted_user", + "ingested": "dynamic", "outcome": "unknown" }, "host": { "architecture": "i386", "hostname": "beowulf", - "ip": "127.0.0.1", + "ip": "10.11.12.13", "name": "beowulf", "os": { "platform": "minix" diff --git a/systemtest/datastreams_test.go b/systemtest/datastreams_test.go deleted file mode 100644 index 88aad971761..00000000000 --- a/systemtest/datastreams_test.go +++ /dev/null @@ -1,140 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package systemtest_test - -import ( - "encoding/json" - "fmt" - "io/ioutil" - "strings" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.uber.org/zap/zapcore" - - "github.com/elastic/apm-server/systemtest" - "github.com/elastic/apm-server/systemtest/apmservertest" - "github.com/elastic/apm-server/systemtest/estest" -) - -func TestDataStreamsEnabled(t *testing.T) { - for _, enabled := range []bool{false, true} { - t.Run(fmt.Sprint(enabled), func(t *testing.T) { - systemtest.CleanupElasticsearch(t) - srv := apmservertest.NewUnstartedServer(t) - if enabled { - // Enable data streams. - srv.Config.DataStreams = &apmservertest.DataStreamsConfig{Enabled: true} - srv.Config.Setup = nil - - // Create a data stream index template. - resp, err := systemtest.Elasticsearch.Indices.PutIndexTemplate("apm-data-streams", strings.NewReader(fmt.Sprintf(`{ - "index_patterns": ["traces-apm*", "logs-apm*", "metrics-apm*"], - "data_stream": {}, - "priority": 200, - "template": {"settings": {"number_of_shards": 1, "refresh_interval": "250ms"}} - }`))) - require.NoError(t, err) - body, _ := ioutil.ReadAll(resp.Body) - require.False(t, resp.IsError(), string(body)) - - // Create an API Key which can write to traces-* etc. - // The default APM Server user can only write to apm-*. - // - // NOTE(axw) importantly, this API key lacks privileges - // to manage templates, pipelines, ILM, etc. Enabling - // data streams should disable all automatic setup. - resp, err = systemtest.Elasticsearch.Security.CreateAPIKey(strings.NewReader(fmt.Sprintf(`{ - "name": "%s", - "expiration": "1h", - "role_descriptors": { - "write-apm-data": { - "cluster": ["monitor"], - "index": [ - { - "names": ["traces-*", "metrics-*", "logs-*"], - "privileges": ["write", "create_index"] - } - ] - } - } - }`, t.Name()))) - require.NoError(t, err) - - var apiKeyResponse struct { - ID string - Name string - APIKey string `json:"api_key"` - } - require.NoError(t, json.NewDecoder(resp.Body).Decode(&apiKeyResponse)) - - // Use an API Key to mimic running under Fleet, with limited permissions. - srv.Config.Output.Elasticsearch.Username = "" - srv.Config.Output.Elasticsearch.Password = "" - srv.Config.Output.Elasticsearch.APIKey = fmt.Sprintf("%s:%s", apiKeyResponse.ID, apiKeyResponse.APIKey) - } - require.NoError(t, srv.Start()) - - tracer := srv.Tracer() - tx := tracer.StartTransaction("name", "type") - tx.Duration = time.Second - tx.End() - tracer.Flush(nil) - - result := systemtest.Elasticsearch.ExpectDocs(t, "apm-*,traces-apm*", estest.TermQuery{ - Field: "processor.event", Value: "transaction", - }) - systemtest.ApproveEvents( - t, t.Name(), result.Hits.Hits, - "@timestamp", "timestamp.us", - "trace.id", "transaction.id", - ) - - // Assert there are no unexpected warnings or errors. - for _, record := range srv.Logs.All() { - assert.Condition(t, func() bool { - if record.Level == zapcore.ErrorLevel { - return assert.Equal(t, "Started apm-server with data streams enabled but no active fleet management mode was specified", record.Message) - } - return record.Level < zapcore.WarnLevel - }, "%s: %s", record.Level, record.Message) - } - }) - } -} - -func TestDataStreamsSetupErrors(t *testing.T) { - cfg := apmservertest.DefaultConfig() - cfg.DataStreams = &apmservertest.DataStreamsConfig{Enabled: true} - cfgargs, err := cfg.Args() - require.NoError(t, err) - - test := func(args []string, expected string) { - args = append(args, cfgargs...) - cmd := apmservertest.ServerCommand("setup", args...) - out, err := cmd.CombinedOutput() - require.Error(t, err) - assert.Equal(t, "Exiting: "+expected+"\n", string(out)) - } - - test([]string{}, "index setup must be performed externally when using data streams, by installing the 'apm' integration package") - test([]string{"--index-management"}, "index setup must be performed externally when using data streams, by installing the 'apm' integration package") - test([]string{"--pipelines"}, "index pipeline setup must be performed externally when using data streams, by installing the 'apm' integration package") -} diff --git a/systemtest/fleet_test.go b/systemtest/fleet_test.go index 761bccaad3c..fa5a7afab7d 100644 --- a/systemtest/fleet_test.go +++ b/systemtest/fleet_test.go @@ -20,6 +20,7 @@ package systemtest_test import ( "context" "io/ioutil" + "net/http" "net/url" "testing" "time" @@ -31,6 +32,7 @@ import ( "go.elastic.co/apm/transport" "github.com/elastic/apm-server/systemtest" + "github.com/elastic/apm-server/systemtest/apmservertest" "github.com/elastic/apm-server/systemtest/fleettest" ) @@ -83,17 +85,36 @@ func TestFleetIntegration(t *testing.T) { require.NoError(t, err) // Elastic Agent has started apm-server. Connect to apm-server and send some data, - // and make sure it gets indexed into a data stream. - transport, err := transport.NewHTTPTransport() + // and make sure it gets indexed into a data stream. We override the transport to + // set known metadata. + httpTransport, err := transport.NewHTTPTransport() require.NoError(t, err) - transport.SetServerURL(&url.URL{Scheme: "http", Host: agent.Addrs["8200"]}) - tracer, err := apm.NewTracerOptions(apm.TracerOptions{Transport: transport}) + origTransport := httpTransport.Client.Transport + httpTransport.Client.Transport = roundTripperFunc(func(r *http.Request) (*http.Response, error) { + r.Header.Set("X-Real-Ip", "10.11.12.13") + return origTransport.RoundTrip(r) + }) + httpTransport.SetServerURL(&url.URL{Scheme: "http", Host: agent.Addrs["8200"]}) + tracer, err := apm.NewTracerOptions(apm.TracerOptions{ + Transport: apmservertest.NewFilteringTransport( + httpTransport, + apmservertest.DefaultMetadataFilter{}, + ), + }) require.NoError(t, err) defer tracer.Close() - tracer.StartTransaction("name", "type").End() + + tx := tracer.StartTransaction("name", "type") + tx.Duration = time.Second + tx.End() tracer.Flush(nil) - systemtest.Elasticsearch.ExpectDocs(t, "traces-*", nil) + result := systemtest.Elasticsearch.ExpectDocs(t, "traces-*", nil) + systemtest.ApproveEvents( + t, t.Name(), result.Hits.Hits, + "@timestamp", "timestamp.us", + "trace.id", "transaction.id", + ) } func TestFleetPackageNonMultiple(t *testing.T) { @@ -143,7 +164,16 @@ func initAPMIntegrationPackagePolicyInputs(t *testing.T, packagePolicy *fleettes } } -func getAPMIntegrationPackage(t *testing.T, fleet *fleettest.Client) *fleettest.Package { +func cleanupFleet(t testing.TB, fleet *fleettest.Client) { + cleanupFleetPolicies(t, fleet) + apmPackage := getAPMIntegrationPackage(t, fleet) + if apmPackage.Status == "installed" { + err := fleet.DeletePackage(apmPackage.Name, apmPackage.Version) + require.NoError(t, err) + } +} + +func getAPMIntegrationPackage(t testing.TB, fleet *fleettest.Client) *fleettest.Package { var apmPackage *fleettest.Package packages, err := fleet.ListPackages() require.NoError(t, err) @@ -161,7 +191,7 @@ func getAPMIntegrationPackage(t *testing.T, fleet *fleettest.Client) *fleettest. panic("unreachable") } -func cleanupFleet(t testing.TB, fleet *fleettest.Client) { +func cleanupFleetPolicies(t testing.TB, fleet *fleettest.Client) { apmAgentPolicies, err := fleet.AgentPolicies("ingest-agent-policies.name:apm_systemtest") require.NoError(t, err) if len(apmAgentPolicies) == 0 { @@ -187,3 +217,9 @@ func cleanupFleet(t testing.TB, fleet *fleettest.Client) { require.NoError(t, err) } } + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} diff --git a/systemtest/fleettest/client.go b/systemtest/fleettest/client.go index 5034bd19f87..0f93351f2dd 100644 --- a/systemtest/fleettest/client.go +++ b/systemtest/fleettest/client.go @@ -239,6 +239,17 @@ func (c *Client) Package(name, version string) (*Package, error) { return &result.Response, nil } +// DeletePackage deletes (uninstalls) the package with the given name and version. +func (c *Client) DeletePackage(name, version string) error { + req := c.newFleetRequest("DELETE", "/epm/packages/"+name+"-"+version, nil) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + return consumeResponse(resp, nil) +} + // PackagePolicy returns information about the package policy with the given ID. func (c *Client) PackagePolicy(id string) (*PackagePolicy, error) { resp, err := http.Get(c.fleetURL + "/package_policies/" + id) From 1cc50779bb8e37f1a1f8d493cb3e669221aab77b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 10:37:00 +0200 Subject: [PATCH 48/55] remove sourcemap api key from manifest --- apmpackage/apm/manifest.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/apmpackage/apm/manifest.yml b/apmpackage/apm/manifest.yml index e99c15f177b..77b07b6faea 100644 --- a/apmpackage/apm/manifest.yml +++ b/apmpackage/apm/manifest.yml @@ -120,12 +120,6 @@ policy_templates: required: false show_user: false default: 10000 - - name: sourcemap_api_key - type: text - title: RUM - API Key for Sourcemaps - required: false - description: API Key for sourcemap feature. Enter as : - show_user: false - name: api_key_limit type: integer title: Maximum number of API Keys for Agent authentication From bb2c6c9e4e0d354472d96df2973ff13408653f38 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 11:26:36 +0200 Subject: [PATCH 49/55] use single fleetcfg nil check --- beater/beater.go | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/beater/beater.go b/beater/beater.go index fa2cbc80e77..c45177e05b9 100644 --- a/beater/beater.go +++ b/beater/beater.go @@ -622,34 +622,32 @@ func newTransformConfig(beatInfo beat.Info, cfg *config.Config, fleetCfg *config } func newSourcemapStore(beatInfo beat.Info, cfg config.SourceMapping, fleetCfg *config.Fleet) (*sourcemap.Store, error) { - if fleetmode.Enabled() { + if fleetCfg != nil { var ( c = *http.DefaultClient rt = http.DefaultTransport ) - if fleetCfg != nil { - var tlsConfig *tlscommon.TLSConfig - var err error - if fleetCfg.TLS.IsEnabled() { - if tlsConfig, err = tlscommon.LoadTLSConfig(fleetCfg.TLS); err != nil { - return nil, err - } - } - - // Default for es is 90s :shrug: - timeout := 30 * time.Second - dialer := transport.NetDialer(timeout) - tlsDialer, err := transport.TLSDialer(dialer, tlsConfig, timeout) - if err != nil { + var tlsConfig *tlscommon.TLSConfig + var err error + if fleetCfg.TLS.IsEnabled() { + if tlsConfig, err = tlscommon.LoadTLSConfig(fleetCfg.TLS); err != nil { return nil, err } + } - rt = &http.Transport{ - Proxy: http.ProxyFromEnvironment, - Dial: dialer.Dial, - DialTLS: tlsDialer.Dial, - TLSClientConfig: tlsConfig.ToConfig(), - } + // Default for es is 90s :shrug: + timeout := 30 * time.Second + dialer := transport.NetDialer(timeout) + tlsDialer, err := transport.TLSDialer(dialer, tlsConfig, timeout) + if err != nil { + return nil, err + } + + rt = &http.Transport{ + Proxy: http.ProxyFromEnvironment, + Dial: dialer.Dial, + DialTLS: tlsDialer.Dial, + TLSClientConfig: tlsConfig.ToConfig(), } c.Transport = apmhttp.WrapRoundTripper(rt) From 65ad0793af1cbd9f9ba7877dad405792f3227c6b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 11:26:53 +0200 Subject: [PATCH 50/55] attach context to outgoing fleet request --- sourcemap/fleet_store.go | 2 +- sourcemap/store_test.go | 27 ++++----------------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index c7273fb3cf8..afd3621825c 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -108,7 +108,7 @@ func (f fleetStore) fetch(ctx context.Context, name, version, path string) (stri } req.Header.Add("Authorization", f.apikey) - resp, err := f.c.Do(req) + resp, err := f.c.Do(req.WithContext(ctx)) if err != nil { return "", err } diff --git a/sourcemap/store_test.go b/sourcemap/store_test.go index 4a5eaf7a46d..ed2782d4438 100644 --- a/sourcemap/store_test.go +++ b/sourcemap/store_test.go @@ -162,14 +162,9 @@ func TestFetchTimeout(t *testing.T) { version = "1.0.0" path = "/my/path/to/bundle.js.map" c = http.DefaultClient - waitc = make(chan struct{}) ) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - <-waitc - // zlib compress - wr := zlib.NewWriter(w) - defer wr.Close() - wr.Write([]byte(test.ValidSourcemap)) + <-r.Context().Done() })) defer ts.Close() @@ -196,24 +191,10 @@ func TestFetchTimeout(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) defer cancel() - var wg sync.WaitGroup - for i := 0; i < 2; i++ { - wg.Add(1) - go func() { - consumer, err := store.Fetch(ctx, name, version, path) - if err != nil { - assert.True(t, errors.Is(err, context.DeadlineExceeded)) - atomic.AddInt64(&errs, 1) - close(waitc) - } else { - assert.NotNil(t, consumer) - } - - wg.Done() - }() - } + _, err = store.Fetch(ctx, name, version, path) + assert.True(t, errors.Is(err, context.DeadlineExceeded)) + atomic.AddInt64(&errs, 1) - wg.Wait() assert.Equal(t, int64(1), errs) } From 7711ee42869179aad21ec44a9f95924358f85d17 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 11:32:26 +0200 Subject: [PATCH 51/55] test sourcemap url path --- sourcemap/fleet_store_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go index 39dada12e76..e3f581a60ea 100644 --- a/sourcemap/fleet_store_test.go +++ b/sourcemap/fleet_store_test.go @@ -31,16 +31,18 @@ import ( func TestFleetFetch(t *testing.T) { var ( - hasAuth bool - apikey = "supersecret" - name = "webapp" - version = "1.0.0" - path = "/my/path/to/bundle.js.map" - wantRes = "sourcemap response" - c = http.DefaultClient + hasAuth bool + apikey = "supersecret" + name = "webapp" + version = "1.0.0" + path = "/my/path/to/bundle.js.map" + wantRes = "sourcemap response" + c = http.DefaultClient + sourceMapPath = "/api/fleet/artifact" ) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, sourceMapPath, r.URL.Path) auth := r.Header.Get("Authorization") hasAuth = auth == "ApiKey "+apikey // zlib compress @@ -62,7 +64,7 @@ func TestFleetFetch(t *testing.T) { ServiceName: name, ServiceVersion: version, BundleFilepath: path, - SourceMapURL: "/", + SourceMapURL: sourceMapPath, }, } fb, err := newFleetStore(c, fleetCfg, cfgs) From 051baa84c8fd09a4bf4446ffc194768a7f141610 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Tue, 22 Jun 2021 15:49:51 +0200 Subject: [PATCH 52/55] remove duplicate rum enable field --- apmpackage/apm/agent/input/template.yml.hbs | 1 - 1 file changed, 1 deletion(-) diff --git a/apmpackage/apm/agent/input/template.yml.hbs b/apmpackage/apm/agent/input/template.yml.hbs index 138867e1589..c916a00c05f 100644 --- a/apmpackage/apm/agent/input/template.yml.hbs +++ b/apmpackage/apm/agent/input/template.yml.hbs @@ -13,7 +13,6 @@ apm-server: read_timeout: {{read_timeout}} response_headers: {{response_headers}} rum: - enabled: {{enable_rum}} allow_headers: {{#each rum_allow_headers}} - {{this}} From 85193f52e48ecc929620f58d0a8c7f08646ca38e Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 23 Jun 2021 09:23:16 +0200 Subject: [PATCH 53/55] update fleet hosts TODO --- sourcemap/fleet_store.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sourcemap/fleet_store.go b/sourcemap/fleet_store.go index afd3621825c..f2108435952 100644 --- a/sourcemap/fleet_store.go +++ b/sourcemap/fleet_store.go @@ -72,9 +72,8 @@ func newFleetStore( fleetCfg *config.Fleet, cfgs []config.SourceMapMetadata, ) (fleetStore, error) { - // TODO(stn): This is assuming we'll get a single fleet address right now. - // If we get more, how do we know which artifact is where? Are they - // present on all fleet-servers and we should rr through the addresses? + // TODO(stn): Add support for multiple fleet hosts + // cf. https://github.com/elastic/apm-server/issues/5514 host := fleetCfg.Hosts[0] fleetURLs := make(map[key]string) From 77fc0a0419b3a0ac6db5b3c53f74ef996793425b Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 23 Jun 2021 09:53:51 +0200 Subject: [PATCH 54/55] verify that fleet store correctly uses Protocol --- sourcemap/fleet_store_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sourcemap/fleet_store_test.go b/sourcemap/fleet_store_test.go index e3f581a60ea..66b36340e96 100644 --- a/sourcemap/fleet_store_test.go +++ b/sourcemap/fleet_store_test.go @@ -53,8 +53,8 @@ func TestFleetFetch(t *testing.T) { defer ts.Close() fleetCfg := &config.Fleet{ - Hosts: []string{ts.URL}, - Protocol: "https", + Hosts: []string{ts.URL[7:]}, + Protocol: "http", AccessAPIKey: apikey, TLS: nil, } From 35468ac9ebefbfe74a19263ca124b94c89bb8238 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 23 Jun 2021 09:54:17 +0200 Subject: [PATCH 55/55] show fleet used if fleetcfg != nil --- beater/beater_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/beater/beater_test.go b/beater/beater_test.go index 7aade29c9f7..b7295aac0c0 100644 --- a/beater/beater_test.go +++ b/beater/beater_test.go @@ -18,6 +18,7 @@ package beater import ( + "compress/zlib" "context" "errors" "net" @@ -306,3 +307,40 @@ func TestStoreUsesRUMElasticsearchConfig(t *testing.T) { assert.True(t, called) } + +func TestFleetStoreUsed(t *testing.T) { + var called bool + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + wr := zlib.NewWriter(w) + defer wr.Close() + wr.Write([]byte(test.ValidSourcemap)) + })) + defer ts.Close() + + cfg := config.DefaultConfig() + cfg.RumConfig.Enabled = true + cfg.RumConfig.SourceMapping.Enabled = true + cfg.RumConfig.SourceMapping.Metadata = []config.SourceMapMetadata{{ + ServiceName: "app", + ServiceVersion: "1.0", + BundleFilepath: "/bundle/path", + SourceMapURL: "/my/path", + }} + + fleetCfg := &config.Fleet{ + Hosts: []string{ts.URL[7:]}, + Protocol: "http", + AccessAPIKey: "my-key", + TLS: nil, + } + + transformConfig, err := newTransformConfig(beat.Info{Version: "1.2.3"}, cfg, fleetCfg) + require.NoError(t, err) + // Check that the provided rum elasticsearch config was used and + // Fetch() goes to the test server. + _, err = transformConfig.RUM.SourcemapStore.Fetch(context.Background(), "app", "1.0", "/bundle/path") + require.NoError(t, err) + + assert.True(t, called) +}