Skip to content

feat: Regex filters for P2P preheat (phase 3) #754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Vad1mo wants to merge 1 commit into
base: feat/regex-filters-phase2
Choose a base branch
from
Open

feat: Regex filters for P2P preheat (phase 3) #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 68 additions & 0 deletions src/controller/p2p/preheat/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/stretchr/testify/suite"

"github.com/goharbor/harbor/src/lib/config"
harborerrors "github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/lib/orm"
"github.com/goharbor/harbor/src/lib/q"
"github.com/goharbor/harbor/src/pkg/p2p/preheat/models/policy"
Expand Down Expand Up @@ -253,6 +254,73 @@ func (s *preheatSuite) TestCreatePolicy() {
s.False(policy.UpdatedTime.IsZero())
}

func (s *preheatSuite) TestPolicyFilterKindValidation() {
cases := []struct {
name string
filtersStr string
wantErr bool
}{
{
name: "no kind",
filtersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`,
},
{
name: "regex kind",
filtersStr: `[{"type":"repository","value":"harbor.*","kind":"regex"},{"type":"tag","value":"2.*","kind":"regex"}]`,
},
{
name: "unknown kind",
filtersStr: `[{"type":"repository","value":"harbor*","kind":"glob"}]`,
wantErr: true,
},
{
name: "invalid regex",
filtersStr: `[{"type":"repository","value":"foo)|(?:bar","kind":"regex"}]`,
wantErr: true,
},
{
name: "kind on a label filter",
filtersStr: `[{"type":"label","value":"prod","kind":"regex"}]`,
wantErr: true,
},
}

stored := &policy.Schema{ID: 99, Name: "test-filter-kind", Trigger: &policy.Trigger{Type: policy.TriggerTypeManual}}
s.fakePolicyMgr.On("Get", s.ctx, int64(99)).Return(stored, nil)

for _, c := range cases {
s.Run(c.name, func() {
p := &policy.Schema{
ID: 99,
Name: "test-filter-kind",
FiltersStr: c.filtersStr,
TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{}}`, policy.TriggerTypeManual),
}
s.fakePolicyMgr.On("Create", s.ctx, p).Return(int64(99), nil).Maybe()
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
s.fakePolicyMgr.On("Update", s.ctx, p, mock.Anything).Return(nil).Maybe()

_, createErr := s.controller.CreatePolicy(s.ctx, p)
updateErr := s.controller.UpdatePolicy(s.ctx, p, "")
if !c.wantErr {
s.NoError(createErr)
s.NoError(updateErr)
// the policy of this case is a distinct pointer, so the recorded calls
// of the other tests in the suite never satisfy these
s.fakePolicyMgr.AssertCalled(s.T(), "Create", s.ctx, p)
s.fakePolicyMgr.AssertCalled(s.T(), "Update", s.ctx, p, mock.Anything)
return
}
s.Error(createErr)
s.True(harborerrors.IsErr(createErr, harborerrors.BadRequestCode), "create error is a bad request: %v", createErr)
s.Error(updateErr)
s.True(harborerrors.IsErr(updateErr, harborerrors.BadRequestCode), "update error is a bad request: %v", updateErr)
// validation runs before persistence, so a rejected policy is never stored
s.fakePolicyMgr.AssertNotCalled(s.T(), "Create", s.ctx, p)
s.fakePolicyMgr.AssertNotCalled(s.T(), "Update", s.ctx, p, mock.Anything)
})
}
}

func (s *preheatSuite) TestGetPolicy() {
s.fakePolicyMgr.On("Get", s.ctx, int64(1)).Return(&policy.Schema{Name: "test"}, nil)
p, err := s.controller.GetPolicy(s.ctx, 1)
Expand Down
60 changes: 60 additions & 0 deletions src/pkg/p2p/preheat/models/policy/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/goharbor/harbor/src/common/utils"
"github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/lib/pattern"
"github.com/goharbor/harbor/src/lib/q"
)

Expand All @@ -48,6 +49,12 @@ const (
// FilterTypeLabel represents the label filter type
FilterTypeLabel FilterType = "label"

// FilterKindDoublestar interprets the filter value as a doublestar pattern, the default
FilterKindDoublestar = pattern.KindDoublestar
// FilterKindRegex interprets the filter value as a regular expression matching the whole
// value, the same engine the retention, immutability and replication filters use
FilterKindRegex = pattern.KindRegex

// TriggerTypeManual represents the manual trigger type
TriggerTypeManual TriggerType = "manual"
// TriggerTypeScheduled represents the scheduled trigger type
Expand Down Expand Up @@ -103,6 +110,50 @@ type FilterType = string
type Filter struct {
Type FilterType `json:"type"`
Value any `json:"value"`
// Kind selects the pattern engine used for Value, empty means FilterKindDoublestar.
// Only the repository and tag filters carry a pattern, the others are exact or numeric.
Kind string `json:"kind,omitempty"`

@cubic-dev-ai cubic-dev-ai Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: A policy stored with kind=regex carries a regex value inside the opaque FiltersStr, but any consumer that does not know the kind key (an older Harbor after downgrade, or harbor-cli/terraform reading/round-tripping the filters JSON) drops it and silently reinterprets the regex as a doublestar glob, changing which artifacts match with no error. The value is only routed by buildFilter's f.Kind == FilterKindRegex check, so there is no boundary enforcement. Consider documenting the downgrade behavior, or at minimum flagging in release notes that downgrading reinterprets regex filters.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/pkg/p2p/preheat/models/policy/policy.go, line 115:

<comment>A policy stored with kind=regex carries a regex value inside the opaque FiltersStr, but any consumer that does not know the `kind` key (an older Harbor after downgrade, or harbor-cli/terraform reading/round-tripping the filters JSON) drops it and silently reinterprets the regex as a doublestar glob, changing which artifacts match with no error. The value is only routed by buildFilter's `f.Kind == FilterKindRegex` check, so there is no boundary enforcement. Consider documenting the downgrade behavior, or at minimum flagging in release notes that downgrading reinterprets regex filters.</comment>

<file context>
@@ -103,6 +110,50 @@ type FilterType = string
 	Value any        `json:"value"`
+	// Kind selects the pattern engine used for Value, empty means FilterKindDoublestar.
+	// Only the repository and tag filters carry a pattern, the others are exact or numeric.
+	Kind string `json:"kind,omitempty"`
+}
+
</file context>

@Vad1mo Vad1mo Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Known and deliberate, no change. The downgrade behavior is documented in the compatibility section of the proposal this stack implements (goharbor/community#298), which covers every filter that gained a kind, not just preheat. Repeating it as a per-PR release-note line would duplicate that and would say it in the one place a downgrading operator is least likely to read.

Enforcing it at the boundary is not possible either: the filters travel as an opaque JSON string end to end, so an older Harbor decodes them with a struct that has no kind field, and nothing this version writes can make that decode fail.

}

// supportsKind reports whether the filter type evaluates its value as a pattern
func (f *Filter) supportsKind() bool {
return f.Type == FilterTypeRepository || f.Type == FilterTypeTag
}

// Validate checks the pattern engine selection of the filter. The value type checks stay
// in the filter builder, which is also reached by the policies stored before this existed.
func (f *Filter) Validate() error {
if f.Kind == "" {
return nil
}

if f.Kind != FilterKindDoublestar && f.Kind != FilterKindRegex {
return errors.New(nil).WithCode(errors.BadRequestCode).
WithMessagef("invalid filter kind: %s", f.Kind)
}

if !f.supportsKind() {
return errors.New(nil).WithCode(errors.BadRequestCode).
WithMessagef("only the %s and %s filters support kind, got: %s",
FilterTypeRepository, FilterTypeTag, f.Type)
}

if f.Kind != FilterKindRegex {
return nil
}

value, ok := f.Value.(string)
if !ok {
return errors.New(nil).WithCode(errors.BadRequestCode).
WithMessagef("the value of the %s filter isn't a string", f.Type)
}

if err := pattern.ValidateRegex(value); err != nil {
return errors.New(nil).WithCode(errors.BadRequestCode).
WithMessagef("invalid regex filter value %q: %v", value, err)
}

return nil
}

// TriggerType represents the type of trigger.
Expand All @@ -120,6 +171,15 @@ type Trigger struct {

// ValidatePreheatPolicy validate preheat policy
func (s *Schema) ValidatePreheatPolicy() error {
for _, filter := range s.Filters {
if filter == nil {
continue
}
if err := filter.Validate(); err != nil {
return err
}
}

// currently only validate cron string of preheat policy
if s.Trigger != nil && s.Trigger.Type == TriggerTypeScheduled && len(s.Trigger.Settings.Cron) > 0 {
if err := utils.ValidateCronString(s.Trigger.Settings.Cron); err != nil {
Expand Down
121 changes: 121 additions & 0 deletions src/pkg/p2p/preheat/models/policy/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
package policy

import (
"strings"
"testing"

"github.com/stretchr/testify/suite"

"github.com/goharbor/harbor/src/lib/errors"
)

// PolicyTestSuite is a test suite for policy schema.
Expand Down Expand Up @@ -66,6 +69,124 @@ func (p *PolicyTestSuite) TestValidatePreheatPolicy() {
p.NoError(p.schema.ValidatePreheatPolicy())
}

// TestValidateFilterKind tests the pattern engine validation of the policy filters
func (p *PolicyTestSuite) TestValidateFilterKind() {
cases := []struct {
name string
filter *Filter
wantErr bool
}{
{
name: "absent kind",
filter: &Filter{Type: FilterTypeRepository, Value: "**"},
},
{
name: "explicit doublestar kind",
filter: &Filter{Type: FilterTypeTag, Value: "prod*", Kind: FilterKindDoublestar},
},
{
name: "regex repository",
filter: &Filter{Type: FilterTypeRepository, Value: "library/.*", Kind: FilterKindRegex},
},
{
name: "regex tag",
filter: &Filter{Type: FilterTypeTag, Value: `v\d+\.\d+`, Kind: FilterKindRegex},
},
{
name: "empty regex pattern",
filter: &Filter{Type: FilterTypeTag, Value: "", Kind: FilterKindRegex},
},
{
name: "unknown kind",
filter: &Filter{Type: FilterTypeTag, Value: "**", Kind: "glob"},
wantErr: true,
},
{
name: "kind on a label filter",
filter: &Filter{Type: FilterTypeLabel, Value: "prod", Kind: FilterKindRegex},
wantErr: true,
},
{
name: "kind on a signature filter",
filter: &Filter{Type: FilterTypeSignature, Value: true, Kind: FilterKindDoublestar},
wantErr: true,
},
{
name: "kind on a vulnerability filter",
filter: &Filter{Type: FilterTypeVulnerability, Value: 3, Kind: FilterKindRegex},
wantErr: true,
},
{
name: "invalid regex",
filter: &Filter{Type: FilterTypeTag, Value: "[", Kind: FilterKindRegex},
wantErr: true,
},
{
name: "regex escaping the anchoring",
filter: &Filter{Type: FilterTypeTag, Value: "foo)|(?:bar", Kind: FilterKindRegex},
wantErr: true,
},
{
name: "regex longer than the pattern limit",
filter: &Filter{Type: FilterTypeTag, Value: strings.Repeat("a", 513), Kind: FilterKindRegex},
wantErr: true,
},
{
name: "regex at the pattern limit",
filter: &Filter{Type: FilterTypeTag, Value: strings.Repeat("a", 512), Kind: FilterKindRegex},
},
{
name: "regex value that isn't a string",
filter: &Filter{Type: FilterTypeTag, Value: 100, Kind: FilterKindRegex},
wantErr: true,
},
}

for _, c := range cases {
p.Run(c.name, func() {
s := &Schema{Filters: []*Filter{c.filter}, Trigger: &Trigger{Type: TriggerTypeManual}}
err := s.ValidatePreheatPolicy()
if !c.wantErr {
p.NoError(err)
return
}
p.Error(err)
p.True(errors.IsErr(err, errors.BadRequestCode), "error is a bad request: %v", err)
})
}
}

// TestFilterKindRoundTrip tests that the kind survives an encode/decode cycle and that a
// policy stored without a kind keeps decoding
func (p *PolicyTestSuite) TestFilterKindRoundTrip() {
s := &Schema{
Filters: []*Filter{
{Type: FilterTypeRepository, Value: "library/.*", Kind: FilterKindRegex},
{Type: FilterTypeTag, Value: "**"},
},
Trigger: &Trigger{Type: TriggerTypeManual},
}
p.NoError(s.Encode())
p.Equal(`[{"type":"repository","value":"library/.*","kind":"regex"},{"type":"tag","value":"**"}]`, s.FiltersStr)

// a policy stored before the kind existed decodes into the doublestar default
stored := &Schema{
FiltersStr: `[{"type":"repository","value":"**"},{"type":"tag","value":"**"},{"type":"label","value":"test"}]`,
TriggerStr: `{"type":"manual","trigger_setting":{"cron":""}}`,
}
p.NoError(stored.Decode())
p.Len(stored.Filters, 3)
for _, f := range stored.Filters {
p.Empty(f.Kind)
}
p.NoError(stored.ValidatePreheatPolicy())

decoded := &Schema{FiltersStr: s.FiltersStr, TriggerStr: `{"type":"manual","trigger_setting":{"cron":""}}`}
p.NoError(decoded.Decode())
p.Equal(FilterKindRegex, decoded.Filters[0].Kind)
p.Empty(decoded.Filters[1].Kind)
}

// TestDecode tests decode.
func (p *PolicyTestSuite) TestDecode() {
s := &Schema{
Expand Down
13 changes: 13 additions & 0 deletions src/pkg/p2p/preheat/policy/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/goharbor/harbor/src/lib/selector"
"github.com/goharbor/harbor/src/lib/selector/selectors/doublestar"
"github.com/goharbor/harbor/src/lib/selector/selectors/label"
regexpselector "github.com/goharbor/harbor/src/lib/selector/selectors/regexp"
"github.com/goharbor/harbor/src/lib/selector/selectors/severity"
"github.com/goharbor/harbor/src/lib/selector/selectors/signature"
"github.com/goharbor/harbor/src/pkg/p2p/preheat/models/policy"
Expand Down Expand Up @@ -152,6 +153,12 @@ func buildFilter(f *policy.Filter) (selector.Selector, error) {
return nil, errors.Errorf("pattern value is missing for filter: %s", f.Type)
}

// Backstop for the write time validation: a policy stored by an older version, or
// written past the API, still has to fail here instead of building a broken selector.
if err := f.Validate(); err != nil {

@cubic-dev-ai cubic-dev-ai Bot Aug 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: buildFilter now calls f.Validate() on every filter build, and for a regex kind that recompiles the expression (ValidateRegex -> CompileRegex, which itself compiles twice) on every enforcer run. The regexpselector matcher then compiles the same expression a third time on first use. This is redundant compile work on the preheat execution path with no new safety (write-time validation already covers valid API-created policies; legacy stored policies have empty Kind and return nil immediately). Consider skipping Validate on the runtime path, or having buildFilter rely on the matcher's own cached compile error, so valid policies are not compiled twice.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/pkg/p2p/preheat/policy/filter.go, line 158:

<comment>`buildFilter` now calls `f.Validate()` on every filter build, and for a regex kind that recompiles the expression (`ValidateRegex` -> `CompileRegex`, which itself compiles twice) on every enforcer run. The `regexpselector` matcher then compiles the same expression a third time on first use. This is redundant compile work on the preheat execution path with no new safety (write-time validation already covers valid API-created policies; legacy stored policies have empty Kind and return nil immediately). Consider skipping Validate on the runtime path, or having buildFilter rely on the matcher's own cached compile error, so valid policies are not compiled twice.</comment>

<file context>
@@ -152,6 +153,12 @@ func buildFilter(f *policy.Filter) (selector.Selector, error) {
 
+	// Backstop for the write time validation: a policy stored by an older version, or
+	// written past the API, still has to fail here instead of building a broken selector.
+	if err := f.Validate(); err != nil {
+		return nil, err
+	}
</file context>

@Vad1mo Vad1mo Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deliberate, no change. The Validate call in buildFilter is a backstop for the two paths that do not go through write-time validation: a policy stored before this version existed, and anything written past the API. Runtime keeping its own check is the point of it.

The cost is not on the hot path either. buildFilter runs once per filter per enforcer run, not per candidate — the compiled expression then lives in the selector's Matcher and is reused across every artifact the run evaluates. So this is one extra regexp.Compile of a pattern capped at 512 characters per policy execution.

return nil, err
}

// Current value type
cvt := reflect.TypeOf(f.Value).Name()

Expand All @@ -176,8 +183,14 @@ func buildFilter(f *policy.Filter) (selector.Selector, error) {
// Build selectors
switch f.Type {
case policy.FilterTypeRepository:
if f.Kind == policy.FilterKindRegex {
return regexpselector.New(regexpselector.RepoMatches, f.Value, ""), nil
}
return doublestar.New(doublestar.RepoMatches, f.Value, ""), nil
case policy.FilterTypeTag:
if f.Kind == policy.FilterKindRegex {
return regexpselector.New(regexpselector.Matches, f.Value, ""), nil
}
return doublestar.New(doublestar.Matches, f.Value, ""), nil
case policy.FilterTypeLabel:
return label.New(label.With, f.Value, ""), nil
Expand Down
Loading