Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 50 additions & 6 deletions extensions/composer/waf/waf.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
package waf

import (
"fmt"
"io"
"net"
"strconv"
"strings"
Expand All @@ -26,18 +28,50 @@ const (
metadataKeyBlockPhase = "block_phase"
)

// wafRegistry tracks all WAF instances created for a single Envoy config version
// (main config + per-route overrides) so they can be closed together in OnDestroy
// to release memoized regex patterns from Coraza's global cache.
type wafRegistry struct {
wafs []coraza.WAF
}

func (r *wafRegistry) add(w coraza.WAF) {
if w != nil {
r.wafs = append(r.wafs, w)
}
}

func (r *wafRegistry) closeAll(l *logger.Logger) {
for _, w := range r.wafs {
if c, ok := w.(io.Closer); ok {
if err := c.Close(); err != nil {
l.Error(fmt.Sprintf("waf: error closing WAF instance: %v", err))
}
}
}
}
Comment on lines +44 to +52

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per route level waf will have completely independent lifetime with waf configuration in factory. So, I guess it's may not be the correct pattern to call Close() method of route level WAF when the OnDesotry() is called.

And when the configuration is updating, the new filter factory will be created first, then the OnDestory() of old filter factory will be called. That's say the current solution will also call the Close() of new created WAF. I am not sure this is the correct behavior.


type wafPluginFactory struct {
shared.EmptyHttpFilterFactory
config coraza.WAF
mode waf.WAFMode
metrics *metrics
config coraza.WAF
mode waf.WAFMode
metrics *metrics
registry *wafRegistry
}

type perRouteWafPluginConfig struct {
config coraza.WAF
mode waf.WAFMode
}

// OnDestroy releases memoized regex caches held by all WAF instances (main + per-route)
// created by this factory. Called by Envoy when the factory is destroyed (e.g. config update).
func (f *wafPluginFactory) OnDestroy() {
l := logger.GetLogger()
l.Sugar().Infof("waf: closing %d WAF instances", len(f.registry.wafs))
f.registry.closeAll(l)
}

func (f *wafPluginFactory) Create(handle shared.HttpFilterHandle) shared.HttpFilter {
config := f.config
mode := f.mode
Expand Down Expand Up @@ -66,6 +100,9 @@ func (f *wafPluginFactory) Create(handle shared.HttpFilterHandle) shared.HttpFil

type wafPluginConfigFactory struct {
shared.EmptyHttpFilterConfigFactory
// registry is shared between the config factory and the plugin factory it creates,
// so per-route WAFs added after the factory is created can still be closed on destroy.
registry *wafRegistry
}

func (f *wafPluginConfigFactory) Create(
Expand All @@ -88,10 +125,14 @@ func (f *wafPluginConfigFactory) Create(
handle.Log(shared.LogLevelInfo, "waf: empty filter config")
}

f.registry = &wafRegistry{}
f.registry.add(wafConfig)

return &wafPluginFactory{
config: wafConfig,
mode: mode,
metrics: newMetrics(handle),
config: wafConfig,
mode: mode,
metrics: newMetrics(handle),
registry: f.registry,
}, nil
}

Expand All @@ -100,6 +141,9 @@ func (f *wafPluginConfigFactory) CreatePerRoute(unparsedConfig []byte) (any, err
if err != nil {
return nil, err
}
if f.registry != nil {
f.registry.add(wafConfig)
}
return &perRouteWafPluginConfig{
config: wafConfig,
mode: mode,
Expand Down
74 changes: 74 additions & 0 deletions extensions/composer/waf/waf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ package waf

import (
"encoding/json"
"fmt"
"io"
"runtime"
"runtime/debug"
"strconv"
"testing"

Expand Down Expand Up @@ -1901,3 +1904,74 @@ func Test_PerRouteConfigBlocksRequest(t *testing.T) {

wafPlugin.OnStreamComplete()
}

// Verifies that OnDestroy() keeps heap growth bounded across
// config reloads by releasing Coraza's memoized regex patterns.
func Test_OnDestroyReleasesMemory(t *testing.T) {
const (
reloads = 30
rulesPerWAF = 50
)

ruleID := 1
makeDirectives := func() []string {
directives := []string{"SecRuleEngine On"}
for range rulesPerWAF {
directives = append(directives, fmt.Sprintf(
`SecRule REQUEST_URI "@rx ^/reload/%d/path$" "id:%d,phase:1,pass,nolog"`, ruleID, ruleID,
))
ruleID++
}
return directives
}

heapAlloc := func() uint64 {
// GC twice then scavenge: the first GC collects most objects, the second
// collects anything promoted or finalized during the first pass, and
// FreeOSMemory forces the runtime to return freed pages to the OS so that
// HeapAlloc reflects only genuinely live allocations.
runtime.GC()
runtime.GC()
debug.FreeOSMemory()
var m runtime.MemStats
runtime.ReadMemStats(&m)
return m.HeapAlloc
}

baseline := heapAlloc()

for range reloads {
// Use a fresh controller per reload so gomock state doesn't accumulate
// and skew the heap measurement.
ctrl := gomock.NewController(t)

mainConfig, err := json.Marshal(map[string]any{"directives": makeDirectives(), "mode": "FULL"})
require.NoError(t, err)
perRouteConfig, err := json.Marshal(map[string]any{"directives": makeDirectives(), "mode": "FULL"})
require.NoError(t, err)

configHandle := mocks.NewMockHttpFilterConfigHandle(ctrl)
configHandle.EXPECT().DefineCounter("waf_tx_total").Return(shared.MetricID(1), shared.MetricsSuccess)
configHandle.EXPECT().DefineCounter("waf_tx_blocked", "authority", "phase", "rule_id").Return(shared.MetricID(2), shared.MetricsSuccess)

configFactory := &wafPluginConfigFactory{}
factory, err := configFactory.Create(configHandle, mainConfig)
require.NoError(t, err)
_, err = configFactory.CreatePerRoute(perRouteConfig)
require.NoError(t, err)

// Simulate Envoy destroying the old factory when a config reload arrives.
factory.OnDestroy()
ctrl.Finish()
}

const maxGrowthKB = uint64(1024)
after := heapAlloc()
heapGrowthKB := (max(after, baseline) - baseline) / 1024

// Without OnDestroy releasing memoized caches: 30 reloads × 2 WAFs × 50 unique
// regexes leaks ~11+ MB. With Close(), heap growth stays under 1 MB.
assert.LessOrEqual(t, heapGrowthKB, maxGrowthKB,
"heap grew %d KB after %d reloads, expected ≤ %d KB: OnDestroy must call Close() on all WAF instances to release Coraza's memoized regex cache",
heapGrowthKB, reloads, maxGrowthKB)
}