From c9dddce135d1a472836aae9e1d7660ab65e58a1d Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 18:10:04 +0100 Subject: [PATCH 1/5] perf: compile regex patterns once instead of per call Several hot paths recompiled the same regex patterns on every call, so filtering or scanning N items compiled the same patterns N times. - filters: ResourceFilterOptions.Compile returns a CompiledResourceFilter, which the AWS (Lambda, ECS clusters and services), Kubernetes and Cloud Run snapshot paths now build once per snapshot instead of once per resource name. Filtering 100 names against two patterns drops from ~348,000 to ~25,000 ns/op and from 5,000 to 108 allocs/op. - digest: the constant SHA256 fingerprint pattern is compiled at package level rather than on every ValidateDigest call. - jira: the default issue key pattern and the ^-\d helper are compiled at package level. A pattern built from project keys is still compiled per call, which cannot panic because validateJiraProjectKeys has already rejected any key that could carry regex metacharacters. An invalid pattern is still reported exactly when it was before: only when matching a name reaches it. A name settled by ExcludeNames/IncludeNames, or by an earlier pattern that already matched, is filtered without the invalid pattern being consulted, so a filter that worked before still works. Compiling therefore cannot fail, and ShouldInclude returns the error. Two clean-ups follow from the above: - kube: filterNamespaces matched names in one goroutine per namespace behind a mutex, and carried an error channel plus a cancellable context solely to report a compile failure out of those goroutines. With the patterns compiled up front the goroutine body cannot fail and the remaining work is microseconds of CPU, so it runs in a plain loop and the returned order is now deterministic. The pod fetching in GetPodsData stays concurrent. - gitview: MatchPatternInCommitMessageORBranchName had no production callers and called MustCompile on a caller-supplied pattern, so an invalid pattern would panic instead of returning an error. Removed; its search semantics are covered by the attest jira search-text test. Closes #826 --- cmd/kosli/attestJira.go | 24 ++- cmd/kosli/attestJira_test.go | 41 ++++ cmd/kosli/snapshotCloudRun.go | 11 +- cmd/kosli/snapshotCloudRun_test.go | 24 +++ internal/aws/aws.go | 37 ++-- internal/aws/aws_test.go | 24 ++- internal/aws/ecs_services_test.go | 8 +- internal/digest/digest.go | 13 +- internal/digest/digest_test.go | 18 ++ internal/filters/resourceFilter.go | 119 +++++++++--- internal/filters/resourceFilter_test.go | 236 +++++++++++++++++++++++- internal/gitview/gitView.go | 40 ---- internal/gitview/gitView_test.go | 156 ---------------- internal/jira/jira.go | 32 +++- internal/jira/jira_test.go | 14 ++ internal/kube/kube.go | 47 +---- internal/kube/kube_test.go | 60 ++++-- 17 files changed, 583 insertions(+), 321 deletions(-) diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index 8b22b9760..96289bfa9 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -303,15 +303,7 @@ func (o *attestJiraOptions) run(args []string) error { // Search commit message, branch name, and secondary source for Jira issue keys, // filtering out false positives from multi-segment identifiers like CVE-2026-41284. - searchTexts := []string{commitInfo.Message} - if !o.ignoreBranchMatch { - searchTexts = append(searchTexts, commitInfo.Branch) - } - if o.secondarySource != "" { - searchTexts = append(searchTexts, o.secondarySource) - } - combinedText := strings.Join(searchTexts, "\n") - issueIDs := jira.FindJiraIssueKeys(combinedText, o.projectKeys) + issueIDs := jira.FindJiraIssueKeys(jiraSearchText(commitInfo, o.secondarySource, o.ignoreBranchMatch), o.projectKeys) logger.Debug("Checked for Jira issue references in Git commit %s on branch %s commit message:\n%s", commitInfo.Sha1, commitInfo.Branch, commitInfo.Message) logger.Debug("the following Jira references are found in commit message or branch name: %v", issueIDs) @@ -374,6 +366,20 @@ func (o *attestJiraOptions) run(args []string) error { return wrapAttestationError(err) } +// jiraSearchText joins the texts that are searched for Jira issue keys: the commit +// message, the branch name unless ignoreBranchMatch is set, and the secondary source +// when one is given. +func jiraSearchText(commitInfo *gitview.CommitInfo, secondarySource string, ignoreBranchMatch bool) string { + searchTexts := []string{commitInfo.Message} + if !ignoreBranchMatch { + searchTexts = append(searchTexts, commitInfo.Branch) + } + if secondarySource != "" { + searchTexts = append(searchTexts, secondarySource) + } + return strings.Join(searchTexts, "\n") +} + func (o *attestJiraOptions) validateJiraProjectKeys() error { // According to Jira documentation https://confluence.atlassian.com/adminjiraserver/changing-the-project-key-format-938847081.html // the Jira project key has to start with a capital letter and can then have capital letters numbers and underscore. diff --git a/cmd/kosli/attestJira_test.go b/cmd/kosli/attestJira_test.go index 6676932a2..0fd926802 100644 --- a/cmd/kosli/attestJira_test.go +++ b/cmd/kosli/attestJira_test.go @@ -7,6 +7,7 @@ import ( billy "github.com/go-git/go-billy/v5" git "github.com/go-git/go-git/v5" + "github.com/kosli-dev/cli/internal/gitview" "github.com/kosli-dev/cli/internal/testHelpers" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -356,6 +357,46 @@ func execJiraTestCase(test cmdTestCase, suite *AttestJiraCommandTestSuite) { runTestCmd(suite.T(), []cmdTestCase{test}) } +func TestJiraSearchText(t *testing.T) { + commitInfo := &gitview.CommitInfo{ + BasicCommitInfo: gitview.BasicCommitInfo{ + Message: "EX-1 fix the thing", + Branch: "bugfix/EX-2", + }, + } + for _, tc := range []struct { + name string + secondarySource string + ignoreBranchMatch bool + want string + }{ + { + name: "the commit message and the branch name are searched", + want: "EX-1 fix the thing\nbugfix/EX-2", + }, + { + name: "the branch name is skipped when ignoreBranchMatch is set", + ignoreBranchMatch: true, + want: "EX-1 fix the thing", + }, + { + name: "a secondary source is appended", + secondarySource: "EX-3", + want: "EX-1 fix the thing\nbugfix/EX-2\nEX-3", + }, + { + name: "a secondary source is appended without the branch name", + secondarySource: "EX-3", + ignoreBranchMatch: true, + want: "EX-1 fix the thing\nEX-3", + }, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, jiraSearchText(commitInfo, tc.secondarySource, tc.ignoreBranchMatch)) + }) + } +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestAttestJiraCommandTestSuite(t *testing.T) { diff --git a/cmd/kosli/snapshotCloudRun.go b/cmd/kosli/snapshotCloudRun.go index b5891a827..34fee5e8b 100644 --- a/cmd/kosli/snapshotCloudRun.go +++ b/cmd/kosli/snapshotCloudRun.go @@ -158,6 +158,9 @@ func (o *snapshotCloudRunOptions) run(args []string) error { return err } + // compile the filter patterns once, instead of once per service and job + compiledFilter := o.resourceFilter.Compile() + ctx := context.Background() client, err := newCloudRunClient(ctx, o.resolveNames) if err != nil { @@ -177,21 +180,21 @@ func (o *snapshotCloudRunOptions) run(args []string) error { filteredServices := make([]cloudrun.Service, 0, len(services)) for _, svc := range services { - include, err := o.resourceFilter.ShouldInclude(svc.Name) + included, err := compiledFilter.ShouldInclude(svc.Name) if err != nil { return err } - if include { + if included { filteredServices = append(filteredServices, svc) } } filteredJobs := make([]cloudrun.Job, 0, len(jobs)) for _, job := range jobs { - include, err := o.resourceFilter.ShouldInclude(job.Name) + included, err := compiledFilter.ShouldInclude(job.Name) if err != nil { return err } - if include { + if included { filteredJobs = append(filteredJobs, job) } } diff --git a/cmd/kosli/snapshotCloudRun_test.go b/cmd/kosli/snapshotCloudRun_test.go index faf19fc94..756c66bf8 100644 --- a/cmd/kosli/snapshotCloudRun_test.go +++ b/cmd/kosli/snapshotCloudRun_test.go @@ -158,6 +158,20 @@ func (suite *SnapshotCloudRunTestSuite) TestSnapshotCloudRunCmd() { cmd: fmt.Sprintf(`snapshot cloud-run %s --project p --region r --include-regex "^a" --exclude-regex "^b" %s`, suite.envName, suite.defaultKosliArguments), golden: "Error: only one of --include-regex, --exclude-regex is allowed\n", }, + { + wantError: true, + name: "10 snapshot cloud-run fails if --include-regex is an invalid regex", + cmd: fmt.Sprintf(`snapshot cloud-run %s --project p --region r --include-regex "[invalid" --dry-run %s`, suite.envName, suite.defaultKosliArguments), + // no literal names are set, so filtering the first service reaches the + // invalid pattern and the snapshot fails + goldenRegex: `invalid include name regex pattern \[invalid: error parsing regexp: missing closing \]`, + }, + { + wantError: true, + name: "11 snapshot cloud-run fails if --exclude-regex is an invalid regex", + cmd: fmt.Sprintf(`snapshot cloud-run %s --project p --region r --exclude-regex "[invalid" --dry-run %s`, suite.envName, suite.defaultKosliArguments), + goldenRegex: `invalid exclude name regex pattern \[invalid: error parsing regexp: missing closing \]`, + }, } runTestCmd(suite.T(), tests) @@ -198,6 +212,16 @@ func (suite *SnapshotCloudRunTestSuite) TestSnapshotCloudRunFilter_ExcludeRegex( require.Contains(suite.T(), out, `"serviceName": "beta"`) } +// TestSnapshotCloudRunFilter_ExcludeLiteralShortCircuitsInvalidRegex covers the reporting +// semantics of an invalid pattern: it is reported only when filtering a name reaches it. +// Both stub services are excluded by name, so neither reaches the invalid pattern and the +// snapshot succeeds with nothing in it. +func (suite *SnapshotCloudRunTestSuite) TestSnapshotCloudRunFilter_ExcludeLiteralShortCircuitsInvalidRegex() { + out := suite.runFilteredCmd(`--exclude alpha,beta --exclude-regex "[invalid"`) + require.NotContains(suite.T(), out, `"serviceName": "alpha"`) + require.NotContains(suite.T(), out, `"serviceName": "beta"`) +} + // TestSnapshotCloudRunCmd_HappyPathReportsToServer exercises the full // CLI → local Kosli server roundtrip with the GCP client stubbed: the env is // already created in SetupTest with type "cloud-run", and the command is diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 8766fceb4..7a410e8fe 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -228,8 +228,9 @@ func (staticCreds *AWSStaticCreds) NewECSClient() (*ecs.Client, error) { } // getFilteredLambdaFuncs fetches a filtered set of lambda functions recursively (50 at a time) and returns a list of FunctionConfiguration +// filter is pre-compiled by the caller, so its regex patterns are not re-compiled per function or per page func getFilteredLambdaFuncs(client LambdaAPI, nextMarker *string, allFunctions *[]types.FunctionConfiguration, - filter *filters.ResourceFilterOptions) (*[]types.FunctionConfiguration, error) { + filter *filters.CompiledResourceFilter) (*[]types.FunctionConfiguration, error) { params := &lambda.ListFunctionsInput{} if nextMarker != nil { params.Marker = nextMarker @@ -240,16 +241,15 @@ func getFilteredLambdaFuncs(client LambdaAPI, nextMarker *string, allFunctions * return allFunctions, err } - if len(filter.IncludeNames) == 0 && len(filter.IncludeNamesRegex) == 0 && - len(filter.ExcludeNames) == 0 && len(filter.ExcludeNamesRegex) == 0 { + if !filter.IsSet() { *allFunctions = append(*allFunctions, listFunctionsOutput.Functions...) } else { for _, f := range listFunctionsOutput.Functions { - include, err := filter.ShouldInclude(*f.FunctionName) + included, err := filter.ShouldInclude(*f.FunctionName) if err != nil { return allFunctions, err } - if include { + if included { *allFunctions = append(*allFunctions, f) } } @@ -277,7 +277,10 @@ func (staticCreds *AWSStaticCreds) GetLambdaPackageData(filter *filters.Resource func getLambdaPackageDataFromClient(client LambdaAPI, filter *filters.ResourceFilterOptions) ([]*LambdaData, error) { lambdaData := []*LambdaData{} - filteredFunctions, err := getFilteredLambdaFuncs(client, nil, &[]types.FunctionConfiguration{}, filter) + // compile the filter patterns once, instead of once per function and per page + compiledFilter := filter.Compile() + + filteredFunctions, err := getFilteredLambdaFuncs(client, nil, &[]types.FunctionConfiguration{}, compiledFilter) if err != nil { return lambdaData, err } @@ -577,8 +580,9 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin } // getFilteredECSClusters fetches a filtered set of ECS clusters recursively (50 at a time) and returns a list of ecs Clusters +// clusterFilter is pre-compiled by the caller, so its regex patterns are not re-compiled per cluster or per page func getFilteredECSClusters(client *ecs.Client, allClusters *[]ecsTypes.Cluster, - clusterFilter *filters.ResourceFilterOptions, nextToken *string, logger *logger.Logger) (*[]ecsTypes.Cluster, error) { + clusterFilter *filters.CompiledResourceFilter, nextToken *string, logger *logger.Logger) (*[]ecsTypes.Cluster, error) { params := &ecs.ListClustersInput{} if nextToken != nil { params.NextToken = nextToken @@ -599,11 +603,11 @@ func getFilteredECSClusters(client *ecs.Client, allClusters *[]ecsTypes.Cluster, *allClusters = append(*allClusters, describeClustersOutput.Clusters...) } else { for _, c := range describeClustersOutput.Clusters { - include, err := clusterFilter.ShouldInclude(*c.ClusterName) + included, err := clusterFilter.ShouldInclude(*c.ClusterName) if err != nil { return allClusters, err } - if include { + if included { *allClusters = append(*allClusters, c) } } @@ -633,7 +637,11 @@ func (staticCreds *AWSStaticCreds) GetEcsTasksData(clusterFilter, serviceFilter return allTasksData, fmt.Errorf("failed to create ECS client: %w", err) } - filteredClusters, err := getFilteredECSClusters(client, &[]ecsTypes.Cluster{}, clusterFilter, nil, logger) + // compile the filter patterns once, instead of once per cluster and per service + compiledClusterFilter := clusterFilter.Compile() + compiledServiceFilter := serviceFilter.Compile() + + filteredClusters, err := getFilteredECSClusters(client, &[]ecsTypes.Cluster{}, compiledClusterFilter, nil, logger) if err != nil { return allTasksData, fmt.Errorf("failed to filter ECS clusters: %w", err) } @@ -649,7 +657,7 @@ func (staticCreds *AWSStaticCreds) GetEcsTasksData(clusterFilter, serviceFilter go func(clusterName string) { defer wg.Done() - filteredServices, err := getFilteredECSServicesInCluster(client, clusterName, &[]ecsTypes.Service{}, serviceFilter, nil, logger) + filteredServices, err := getFilteredECSServicesInCluster(client, clusterName, &[]ecsTypes.Service{}, compiledServiceFilter, nil, logger) if err != nil { errChan <- fmt.Errorf("failed to filter ECS services in cluster %s: %w", clusterName, err) return @@ -682,7 +690,8 @@ func (staticCreds *AWSStaticCreds) GetEcsTasksData(clusterFilter, serviceFilter } // getFilteredECSServicesInCluster fetches a filtered set of ECS services recursively (10 at a time) and returns a list of ecs Services -func getFilteredECSServicesInCluster(client ECSServicesAPI, cluster string, allServices *[]ecsTypes.Service, serviceFilter *filters.ResourceFilterOptions, +// serviceFilter is pre-compiled by the caller, so its regex patterns are not re-compiled per service, per page or per cluster +func getFilteredECSServicesInCluster(client ECSServicesAPI, cluster string, allServices *[]ecsTypes.Service, serviceFilter *filters.CompiledResourceFilter, nextToken *string, logger *logger.Logger) (*[]ecsTypes.Service, error) { listInput := &ecs.ListServicesInput{ Cluster: aws.String(cluster), @@ -713,11 +722,11 @@ func getFilteredECSServicesInCluster(client ECSServicesAPI, cluster string, allS *allServices = append(*allServices, describeServicesOutput.Services...) } else { for _, s := range describeServicesOutput.Services { - include, err := serviceFilter.ShouldInclude(*s.ServiceName) + included, err := serviceFilter.ShouldInclude(*s.ServiceName) if err != nil { return allServices, err } - if include { + if included { *allServices = append(*allServices, s) } } diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go index d6d03654e..3b0716ea9 100644 --- a/internal/aws/aws_test.go +++ b/internal/aws/aws_test.go @@ -763,13 +763,35 @@ func (suite *AWSTestSuite) TestGetFilteredLambdaFuncs() { filter: &filters.ResourceFilterOptions{IncludeNamesRegex: []string{"invalid["}}, wantErr: true, }, + { + // an invalid pattern is only reported once filtering a function reaches it, + // so functions settled by ExcludeNames are filtered without error + name: "invalid regex behind an excluded literal name is not reached", + functions: []string{"alpha"}, + filter: &filters.ResourceFilterOptions{ + ExcludeNames: []string{"alpha"}, + ExcludeNamesRegex: []string{"invalid["}, + }, + expectedNames: []string{}, + }, + { + name: "invalid regex is reported once a function does not match the excluded literal name", + functions: []string{"alpha", "beta"}, + filter: &filters.ResourceFilterOptions{ + ExcludeNames: []string{"alpha"}, + ExcludeNamesRegex: []string{"invalid["}, + }, + wantErr: true, + }, } { suite.Run(t.name, func() { client := fakeLambdaClientWithFunctions(t.functions...) if t.pageSize > 0 { client.PageSize = t.pageSize } - result, err := getFilteredLambdaFuncs(client, nil, &[]types.FunctionConfiguration{}, t.filter) + compiledFilter := t.filter.Compile() + + result, err := getFilteredLambdaFuncs(client, nil, &[]types.FunctionConfiguration{}, compiledFilter) if t.wantErr { require.Error(suite.T(), err) return diff --git a/internal/aws/ecs_services_test.go b/internal/aws/ecs_services_test.go index 50807c354..a66c95ae7 100644 --- a/internal/aws/ecs_services_test.go +++ b/internal/aws/ecs_services_test.go @@ -21,11 +21,13 @@ func TestGetFilteredECSServicesInCluster_EmptyCluster(t *testing.T) { Services: []ecsTypes.Service{}, } + emptyFilter := (&filters.ResourceFilterOptions{}).Compile() + allServices, err := getFilteredECSServicesInCluster( client, "empty-cluster", &[]ecsTypes.Service{}, - &filters.ResourceFilterOptions{}, + emptyFilter, nil, logger.NewStandardLogger(), ) @@ -43,11 +45,13 @@ func TestGetFilteredECSServicesInCluster_WithServices(t *testing.T) { Services: []ecsTypes.Service{{ServiceName: &svcName}}, } + emptyFilter := (&filters.ResourceFilterOptions{}).Compile() + allServices, err := getFilteredECSServicesInCluster( client, "cluster", &[]ecsTypes.Service{}, - &filters.ResourceFilterOptions{}, + emptyFilter, nil, logger.NewStandardLogger(), ) diff --git a/internal/digest/digest.go b/internal/digest/digest.go index d065ea4f6..ab09b942a 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -343,14 +343,15 @@ func RemoteDockerImageSha256(client *requests.Client, imageName, imageTag, regis return strings.TrimPrefix(digestHeader, "sha256:"), nil } +const validSha256regex = "^([a-f0-9]{64})$" + +// validSha256Regexp is compiled once, so ValidateDigest does not re-compile a +// constant pattern on every call. +var validSha256Regexp = regexp.MustCompile(validSha256regex) + // ValidateDigest checks if a digest matches the sha256 regex func ValidateDigest(sha256ToCheck string) error { - validSha256regex := "^([a-f0-9]{64})$" - r, err := regexp.Compile(validSha256regex) - if err != nil { - return fmt.Errorf("failed to validate the provided SHA256 fingerprint") - } - if !r.MatchString(sha256ToCheck) { + if !validSha256Regexp.MatchString(sha256ToCheck) { return fmt.Errorf("%s is not a valid SHA256 fingerprint. It should match the pattern %v", sha256ToCheck, validSha256regex) } return nil diff --git a/internal/digest/digest_test.go b/internal/digest/digest_test.go index fcb471b6c..76367a788 100644 --- a/internal/digest/digest_test.go +++ b/internal/digest/digest_test.go @@ -638,6 +638,15 @@ func (suite *DigestTestSuite) TestValidateDigest() { } } +// TestValidateDigestErrorMessage guards the exact error message. Command tests in +// cmd/kosli assert this text (including the regex pattern) in their golden output. +func (suite *DigestTestSuite) TestValidateDigestErrorMessage() { + err := ValidateDigest("xxxx") + require.EqualError(suite.T(), + err, + "xxxx is not a valid SHA256 fingerprint. It should match the pattern ^([a-f0-9]{64})$") +} + func (suite *DigestTestSuite) TestDockerImageSha256() { type want struct { sha256 string @@ -973,3 +982,12 @@ func (suite *DigestTestSuite) TestGetExcludePathsFromIgnoreFile() { func TestDigestTestSuite(t *testing.T) { suite.Run(t, new(DigestTestSuite)) } + +func BenchmarkValidateDigest(b *testing.B) { + sha256 := "db40d79b3a15b17ee9fcc2f49aa73736e0073de6b5a35c459268bb9a31e55139" + for b.Loop() { + if err := ValidateDigest(sha256); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/filters/resourceFilter.go b/internal/filters/resourceFilter.go index 39668fbd5..829eecd58 100644 --- a/internal/filters/resourceFilter.go +++ b/internal/filters/resourceFilter.go @@ -6,6 +6,8 @@ import ( "slices" ) +// ResourceFilterOptions holds the raw, uncompiled filter values as supplied by CLI flags +// or config files. Compile it once before filtering many resource names. type ResourceFilterOptions struct { IncludeNames []string IncludeNamesRegex []string @@ -18,39 +20,110 @@ func (filter *ResourceFilterOptions) IsSet() bool { return len(filter.IncludeNames) > 0 || len(filter.IncludeNamesRegex) > 0 || len(filter.ExcludeNames) > 0 || len(filter.ExcludeNamesRegex) > 0 } +// namePatterns is a list of resource name regex patterns after compilation, split at the +// first pattern that failed to compile: usable holds the patterns before it, which are +// matched in order, and err is the error it produced, reported only once matching a name +// has got past all of usable. Patterns behind it are unreachable and so are dropped, which +// is how they behaved when ShouldInclude compiled patterns on the fly. +type namePatterns struct { + usable []*regexp.Regexp + err error +} + +// isSet reports whether any pattern was supplied, valid or not. +func (patterns namePatterns) isSet() bool { + return len(patterns.usable) > 0 || patterns.err != nil +} + +// match reports whether name matches any of the usable patterns, and errors if name gets +// past all of them and an invalid pattern is waiting behind them. +func (patterns namePatterns) match(name string) (bool, error) { + for _, re := range patterns.usable { + if re.MatchString(name) { + return true, nil + } + } + return false, patterns.err +} + +// CompiledResourceFilter is a ResourceFilterOptions with its regex patterns compiled once, +// so the result can be reused across many resource names without re-compiling per name. +// It is immutable once Compile returns, and is therefore safe for concurrent use. +type CompiledResourceFilter struct { + includeNames []string + includePatterns namePatterns + excludeNames []string + excludePatterns namePatterns +} + +// Compile pre-compiles the include and exclude regex patterns of a filter, so the result +// can be reused across many resource names without re-compiling per name. +// +// Compiling cannot fail: an invalid pattern is kept as the error it produced and is +// reported by ShouldInclude when matching a name reaches that pattern. Reporting it here +// instead would reject filters that never reach it, such as a name settled by +// ExcludeNames, or an invalid pattern behind one that already matched. +func (filter *ResourceFilterOptions) Compile() *CompiledResourceFilter { + return &CompiledResourceFilter{ + includeNames: filter.IncludeNames, + includePatterns: compileNamesRegex(filter.IncludeNamesRegex, "include"), + excludeNames: filter.ExcludeNames, + excludePatterns: compileNamesRegex(filter.ExcludeNamesRegex, "exclude"), + } +} + +// compileNamesRegex compiles a list of resource name regex patterns, stopping at the first +// pattern that fails to compile and keeping its error. +// kind is the name of the filter operation (include, exclude) and is only used in errors. +func compileNamesRegex(patterns []string, kind string) namePatterns { + if len(patterns) == 0 { + return namePatterns{} + } + compiled := make([]*regexp.Regexp, 0, len(patterns)) + for _, pattern := range patterns { + re, err := regexp.Compile(pattern) + if err != nil { + return namePatterns{ + usable: compiled, + err: fmt.Errorf("invalid %s name regex pattern %s: %v", kind, pattern, err), + } + } + compiled = append(compiled, re) + } + return namePatterns{usable: compiled} +} + +// IsSet checks if the filter options are set +func (filter *CompiledResourceFilter) IsSet() bool { + return len(filter.includeNames) > 0 || filter.includePatterns.isSet() || len(filter.excludeNames) > 0 || filter.excludePatterns.isSet() +} + // ShouldInclude checks if a name should be included or not according to the filter options // the filter should only be used for one operation (include, exclude) -func (filter *ResourceFilterOptions) ShouldInclude(name string) (bool, error) { - if len(filter.ExcludeNames) > 0 || len(filter.ExcludeNamesRegex) > 0 { - if slices.Contains(filter.ExcludeNames, name) { +// +// An invalid pattern is reported only when matching this name reaches it, so a name that a +// literal or an earlier pattern already settled is filtered without error. +func (filter *CompiledResourceFilter) ShouldInclude(name string) (bool, error) { + if len(filter.excludeNames) > 0 || filter.excludePatterns.isSet() { + if slices.Contains(filter.excludeNames, name) { return false, nil } - for _, pattern := range filter.ExcludeNamesRegex { - re, err := regexp.Compile(pattern) - if err != nil { - return false, fmt.Errorf("invalid exclude name regex pattern %s: %v", pattern, err) - } - if re.MatchString(name) { - return false, nil - } + excluded, err := filter.excludePatterns.match(name) + if err != nil { + return false, err } - return true, nil - } else if len(filter.IncludeNames) > 0 || len(filter.IncludeNamesRegex) > 0 { + return !excluded, nil + } else if len(filter.includeNames) > 0 || filter.includePatterns.isSet() { // inclusion - if slices.Contains(filter.IncludeNames, name) { + if slices.Contains(filter.includeNames, name) { return true, nil } - for _, pattern := range filter.IncludeNamesRegex { - re, err := regexp.Compile(pattern) - if err != nil { - return false, fmt.Errorf("invalid include name regex pattern %s: %v", pattern, err) - } - if re.MatchString(name) { - return true, nil - } + included, err := filter.includePatterns.match(name) + if err != nil { + return false, err } - return false, nil + return included, nil } return true, nil } diff --git a/internal/filters/resourceFilter_test.go b/internal/filters/resourceFilter_test.go index f11ad5362..fedb3224c 100644 --- a/internal/filters/resourceFilter_test.go +++ b/internal/filters/resourceFilter_test.go @@ -1,6 +1,8 @@ package filters import ( + "fmt" + "sync" "testing" "github.com/stretchr/testify/require" @@ -17,7 +19,7 @@ func (suite *FiltersSuite) TestShouldInclude() { input string filter *ResourceFilterOptions want bool - wantErr bool + wantErr string }{ { name: "returns false when input does not match included", @@ -57,7 +59,7 @@ func (suite *FiltersSuite) TestShouldInclude() { filter: &ResourceFilterOptions{ IncludeNamesRegex: []string{"^foo["}, }, - wantErr: true, + wantErr: "invalid include name regex pattern ^foo[: error parsing regexp: missing closing ]: `[`", }, { name: "returns false when input matches excluded", @@ -97,23 +99,241 @@ func (suite *FiltersSuite) TestShouldInclude() { filter: &ResourceFilterOptions{ ExcludeNamesRegex: []string{"^foo["}, }, - wantErr: true, + wantErr: "invalid exclude name regex pattern ^foo[: error parsing regexp: missing closing ]: `[`", }, } { suite.Run(t.name, func() { - answer, err := t.filter.ShouldInclude(t.input) - require.False(suite.T(), (err != nil) != t.wantErr, - "ShouldInclude() error = %v, wantErr %v", err, t.wantErr) - if !t.wantErr { + included, err := t.filter.Compile().ShouldInclude(t.input) + if t.wantErr != "" { + require.EqualError(suite.T(), err, t.wantErr) + return + } + require.NoError(suite.T(), err) + require.Equal(suite.T(), t.want, included) + }) + } +} + +// TestShouldIncludeReportsInvalidPatternsLazily pins the reporting semantics an invalid +// pattern has always had: it is reported only when matching a name actually reaches it. +// A name settled by a literal or by an earlier pattern never reaches a later invalid one, +// and patterns in the branch that is not taken are never reached at all. +func (suite *FiltersSuite) TestShouldIncludeReportsInvalidPatternsLazily() { + for _, t := range []struct { + name string + input string + filter *ResourceFilterOptions + want bool + wantErr string + }{ + { + name: "an excluded literal name is settled before the invalid pattern is reached", + input: "foo", + filter: &ResourceFilterOptions{ + ExcludeNames: []string{"foo"}, + ExcludeNamesRegex: []string{"^foo["}, + }, + want: false, + }, + { + name: "a name that matches no literal reaches the invalid exclude pattern", + input: "bar", + filter: &ResourceFilterOptions{ + ExcludeNames: []string{"foo"}, + ExcludeNamesRegex: []string{"^foo["}, + }, + wantErr: "invalid exclude name regex pattern ^foo[: error parsing regexp: missing closing ]: `[`", + }, + { + name: "an included literal name is settled before the invalid pattern is reached", + input: "foo", + filter: &ResourceFilterOptions{ + IncludeNames: []string{"foo"}, + IncludeNamesRegex: []string{"^foo["}, + }, + want: true, + }, + { + name: "an earlier matching exclude pattern is reached before the invalid one", + input: "bar1", + filter: &ResourceFilterOptions{ + ExcludeNamesRegex: []string{"^bar.*$", "^foo["}, + }, + want: false, + }, + { + name: "an earlier matching include pattern is reached before the invalid one", + input: "bar1", + filter: &ResourceFilterOptions{ + IncludeNamesRegex: []string{"^bar.*$", "^foo["}, + }, + want: true, + }, + { + name: "an invalid include pattern is never reached while excluding", + input: "bar", + filter: &ResourceFilterOptions{ + ExcludeNames: []string{"foo"}, + IncludeNamesRegex: []string{"^foo["}, + }, + want: true, + }, + { + name: "the first invalid pattern reached is the one reported", + input: "baz", + filter: &ResourceFilterOptions{ + ExcludeNamesRegex: []string{"^foo[", "^bar("}, + }, + wantErr: "invalid exclude name regex pattern ^foo[: error parsing regexp: missing closing ]: `[`", + }, + } { + suite.Run(t.name, func() { + included, err := t.filter.Compile().ShouldInclude(t.input) + if t.wantErr != "" { + require.EqualError(suite.T(), err, t.wantErr) + return + } + require.NoError(suite.T(), err) + require.Equal(suite.T(), t.want, included) + }) + } +} + +func (suite *FiltersSuite) TestCompile() { + for _, t := range []struct { + name string + filter *ResourceFilterOptions + wantIsSet bool + included []string + notIncluded []string + }{ + { + name: "an empty filter includes everything", + filter: &ResourceFilterOptions{}, + wantIsSet: false, + included: []string{"foo", "bar"}, + }, + { + name: "include names and regex patterns are compiled", + filter: &ResourceFilterOptions{ + IncludeNames: []string{"foo"}, + IncludeNamesRegex: []string{"^bar.*$", "^baz.*$"}, + }, + wantIsSet: true, + included: []string{"foo", "bar1", "baz2"}, + notIncluded: []string{"cli-test", "foo1"}, + }, + { + name: "exclude names and regex patterns are compiled", + filter: &ResourceFilterOptions{ + ExcludeNames: []string{"foo"}, + ExcludeNamesRegex: []string{"^bar.*$", "^baz.*$"}, + }, + wantIsSet: true, + included: []string{"cli-test", "foo1"}, + notIncluded: []string{"foo", "bar1", "baz2"}, + }, + { + name: "an invalid pattern still yields a usable filter", + filter: &ResourceFilterOptions{ + ExcludeNames: []string{"foo"}, + ExcludeNamesRegex: []string{"^foo["}, + }, + wantIsSet: true, + notIncluded: []string{"foo"}, + }, + } { + suite.Run(t.name, func() { + compiled := t.filter.Compile() + require.Equal(suite.T(), t.wantIsSet, compiled.IsSet()) + require.Equal(suite.T(), t.filter.IsSet(), compiled.IsSet()) + + for _, name := range t.included { + included, err := compiled.ShouldInclude(name) + require.NoError(suite.T(), err) + require.True(suite.T(), included, "expected %s to be included", name) + } + for _, name := range t.notIncluded { + included, err := compiled.ShouldInclude(name) require.NoError(suite.T(), err) - require.Equal(suite.T(), answer, t.want) + require.False(suite.T(), included, "expected %s to NOT be included", name) } }) } } +// TestCompiledFilterConcurrentShouldInclude verifies that one compiled filter can be +// shared by many goroutines, as the k8s and ECS snapshot paths do. The filter carries an +// invalid pattern so that the error path is exercised concurrently too. Run with -race. +func (suite *FiltersSuite) TestCompiledFilterConcurrentShouldInclude() { + filter := &ResourceFilterOptions{ + IncludeNamesRegex: []string{"^include-.*$", "^never-reached["}, + } + compiled := filter.Compile() + + const workers = 20 + type outcome struct { + included bool + err error + } + results := make([]outcome, workers) + var wg sync.WaitGroup + for i := 0; i < workers; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + // even workers match the first pattern and never reach the invalid one, + // odd workers match nothing and so are reported the invalid pattern + name := fmt.Sprintf("include-%d", idx) + if idx%2 == 1 { + name = fmt.Sprintf("other-%d", idx) + } + included, err := compiled.ShouldInclude(name) + results[idx] = outcome{included: included, err: err} + }(i) + } + wg.Wait() + + for i, result := range results { + if i%2 == 1 { + require.EqualError(suite.T(), result.err, + "invalid include name regex pattern ^never-reached[: error parsing regexp: missing closing ]: `[`") + continue + } + require.NoError(suite.T(), result.err) + require.True(suite.T(), result.included, "expected include-%d to be included", i) + } +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestFiltersSuite(t *testing.T) { suite.Run(t, new(FiltersSuite)) } + +const benchmarkNamesCount = 100 + +func benchmarkNames() []string { + names := make([]string, benchmarkNamesCount) + for i := range names { + names[i] = fmt.Sprintf("prod-namespace-%d", i) + } + return names +} + +// BenchmarkCompiledResourceFilterShouldInclude filters many names with the +// patterns compiled once. +func BenchmarkCompiledResourceFilterShouldInclude(b *testing.B) { + filter := &ResourceFilterOptions{ + IncludeNamesRegex: []string{"^prod-.*$", "^staging-.*$"}, + } + names := benchmarkNames() + for b.Loop() { + compiled := filter.Compile() + for _, name := range names { + if _, err := compiled.ShouldInclude(name); err != nil { + b.Fatal(err) + } + } + } +} diff --git a/internal/gitview/gitView.go b/internal/gitview/gitView.go index e9365817e..08dfed9c9 100644 --- a/internal/gitview/gitView.go +++ b/internal/gitview/gitView.go @@ -3,8 +3,6 @@ package gitview import ( "fmt" "net/url" - "regexp" - "sort" "strings" git "github.com/go-git/go-git/v5" @@ -278,44 +276,6 @@ func getCommitURL(repoURL, commitHash string) string { } } -// MatchPatternInCommitMessageORBranchName returns a slice of strings matching a pattern in a commit message or branch name -// matches lookup happens in the commit message first, and if none is found, matching against the branch name is done -// if no matches are found in both the commit message and the branch name, an empty slice is returned -func (gv *GitView) MatchPatternInCommitMessageORBranchName(pattern, commitSHA, secondarySource string, ignoreBranchMatch bool) ([]string, *CommitInfo, error) { - commitInfo, err := gv.GetCommitInfoFromCommitSHA(commitSHA, true, []string{}) - if err != nil { - return []string{}, nil, err - } - - re := regexp.MustCompile(pattern) - commitMatches := re.FindAllString(commitInfo.Message, -1) - branchMatches := re.FindAllString(commitInfo.Branch, -1) - secondaryMatches := re.FindAllString(secondarySource, -1) - - // Use a map to remove duplicates - uniqueMatches := make(map[string]struct{}) - for _, match := range commitMatches { - uniqueMatches[match] = struct{}{} - } - if !ignoreBranchMatch { - for _, match := range branchMatches { - uniqueMatches[match] = struct{}{} - } - } - for _, match := range secondaryMatches { - uniqueMatches[match] = struct{}{} - } - - // Convert map keys back to a slice - matches := make([]string, 0, len(uniqueMatches)) - for match := range uniqueMatches { - matches = append(matches, match) - } - sort.Strings(matches) - - return matches, commitInfo, nil -} - // ResolveRevision returns an explicit commit SHA1 from commit SHA or ref (e.g. HEAD~2) func (gv *GitView) ResolveRevision(commitSHAOrRef string) (string, error) { hash, err := gv.repository.ResolveRevision(plumbing.Revision(commitSHAOrRef)) diff --git a/internal/gitview/gitView_test.go b/internal/gitview/gitView_test.go index 2ff79040f..8b90f2bf7 100644 --- a/internal/gitview/gitView_test.go +++ b/internal/gitview/gitView_test.go @@ -7,8 +7,6 @@ import ( "path/filepath" "testing" - "github.com/kosli-dev/cli/internal/jira" - "github.com/go-git/go-billy/v5/osfs" git "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing/cache" @@ -384,160 +382,6 @@ func (suite *GitViewTestSuite) TestGetCommitInfoFromCommitSHA() { require.Equal(suite.T(), redactedCommitInfoValue, commitInfo.Branch) } -func (suite *GitViewTestSuite) TestMatchPatternInCommitMessageORBranchName() { - _, workTree, fs, err := testHelpers.InitializeGitRepo(suite.tmpDir) - require.NoError(suite.T(), err) - - defaultJiraPattern := "[A-Z][A-Z0-9]{1,9}-[0-9]+" - for _, t := range []struct { - name string - pattern string - commitMessage string - secondarySource string - ignoreBranchMatch bool - wantError bool - want []string - commitSha string - branchName string - }{ - { - name: "One Jira reference found", - pattern: jira.MakeJiraIssueKeyPattern([]string{}), - commitMessage: "EX-1 test commit", - want: []string{"EX-1"}, - wantError: false, - }, - { - name: "Two Jira references found", - pattern: defaultJiraPattern, - commitMessage: "EX-1 ABC-22 test commit", - want: []string{"EX-1", "ABC-22"}, - wantError: false, - }, - { - name: "No Jira references found", - pattern: defaultJiraPattern, - commitMessage: "test commit", - want: []string{}, - wantError: false, - }, - { - name: "Jira references found in branch name", - pattern: defaultJiraPattern, - commitMessage: "some test commit", - branchName: "EX-5-cool-branch", - want: []string{"EX-5"}, - wantError: false, - }, - { - name: "Jira references found in branch name but ignoreBranchMatch set", - pattern: defaultJiraPattern, - commitMessage: "some test commit", - branchName: "EX-6-cool-branch", - ignoreBranchMatch: true, - want: []string{}, - wantError: false, - }, - { - name: "Jira references found in secondary source", - pattern: defaultJiraPattern, - commitMessage: "some test commit", - secondarySource: "EX-1-test-commit", - want: []string{"EX-1"}, - wantError: false, - }, - { - name: "Jira references found in commit and secondary source", - pattern: defaultJiraPattern, - commitMessage: "EX-1 some test commit", - secondarySource: "EX-2-test-commit", - want: []string{"EX-1", "EX-2"}, - wantError: false, - }, - { - name: "Jira references found in commit and branch name", - pattern: defaultJiraPattern, - commitMessage: "EX-1 some test commit", - branchName: "EX-2-test-commit", - want: []string{"EX-1", "EX-2"}, - wantError: false, - }, - { - name: "Same Jira references found in commit and branch name is not duplicated", - pattern: defaultJiraPattern, - commitMessage: "DUP-1 some test commit", - branchName: "DUP-1-test-commit", - want: []string{"DUP-1"}, - wantError: false, - }, - { - name: "Jira references found in commit, branch name and secondary source", - pattern: defaultJiraPattern, - commitMessage: "ALL-1 some test commit", - branchName: "ALL-2-test-commit", - secondarySource: "ALL-3-some-things", - want: []string{"ALL-1", "ALL-2", "ALL-3"}, - wantError: false, - }, - { - name: "No Jira references found, despite something that looks similar to Jira reference", - pattern: defaultJiraPattern, - commitMessage: "Ea-1 test commit", - want: []string{}, - wantError: false, - }, - { - name: "One Jira reference found with specific pattern", - pattern: jira.MakeJiraIssueKeyPattern([]string{"EX"}), - commitMessage: "EX-1 ABC-22 test commit", - want: []string{"EX-1"}, - wantError: false, - }, - { - name: "Two Jira references found with specific pattern", - pattern: jira.MakeJiraIssueKeyPattern([]string{"EX", "ABC"}), - commitMessage: "EX-1 ABC-22 test commit", - want: []string{"EX-1", "ABC-22"}, - wantError: false, - }, - { - name: "Commit not found, expect an error", - pattern: defaultJiraPattern, - commitSha: "3b7420d0392114794591aaefcd84d7b100b8d095", - wantError: true, - }, - { - name: "GitHub reference found", - pattern: "#[0-9]+", - commitMessage: "#324 test commit", - want: []string{"#324"}, - wantError: false, - }, - } { - suite.Run(t.name, func() { - - if t.commitSha == "" { - t.commitSha, err = testHelpers.CommitToRepo(workTree, fs, t.commitMessage) - require.NoError(suite.T(), err) - } - - if t.branchName != "" { - err := testHelpers.CheckoutNewBranch(workTree, t.branchName) - require.NoError(suite.T(), err) - defer testHelpers.CheckoutMaster(workTree, suite.T()) - } - - gitView, err := New(suite.tmpDir) - require.NoError(suite.T(), err) - - actual, _, err := gitView.MatchPatternInCommitMessageORBranchName(t.pattern, t.commitSha, t.secondarySource, t.ignoreBranchMatch) - require.True(suite.T(), (err != nil) == t.wantError) - require.ElementsMatch(suite.T(), t.want, actual) - - }) - } -} - func (suite *GitViewTestSuite) TestResolveRevision() { _, workTree, fs, err := testHelpers.InitializeGitRepo(suite.tmpDir) require.NoError(suite.T(), err) diff --git a/internal/jira/jira.go b/internal/jira/jira.go index 2ba96445f..af5a6a7fb 100644 --- a/internal/jira/jira.go +++ b/internal/jira/jira.go @@ -107,6 +107,15 @@ func (jc *JiraConfig) GetJiraIssueInfo(issueID string, issueFields string) (*Jir return result, nil } +const defaultJiraIssueKeyPattern = `\b[A-Z][A-Z0-9]{1,9}-[0-9]+` + +var ( + // compiled once, as the default pattern is a constant + defaultJiraIssueKeyRegexp = regexp.MustCompile(defaultJiraIssueKeyPattern) + // dashDigitRegexp is compiled once and shared by all isPartialMultiSegment calls + dashDigitRegexp = regexp.MustCompile(`^-\d`) +) + func MakeJiraIssueKeyPattern(projectKeys []string) string { // Jira issue keys consist of [project-key]-[sequential-number]. // FindJiraIssueKeys uppercases the text before applying this pattern, so the @@ -114,7 +123,7 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { // are also uppercased here for the same reason. // more info: https://support.atlassian.com/jira-software-cloud/docs/what-is-an-issue/#Workingwithissues-Projectandissuekeys if len(projectKeys) == 0 { - return `\b[A-Z][A-Z0-9]{1,9}-[0-9]+` + return defaultJiraIssueKeyPattern } upper := make([]string, len(projectKeys)) for i, k := range projectKeys { @@ -123,6 +132,17 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { return `\b(` + strings.Join(upper, "|") + `)-[0-9]+` } +// jiraIssueKeyRegexp returns the compiled issue key pattern for the given project keys. +// The default pattern is compiled once; a project-key pattern is compiled per call, which +// is safe from panics because validateJiraProjectKeys has already rejected any key outside +// ^[A-Za-z][A-Za-z0-9_]{1,9}$, so a key cannot carry regex metacharacters. +func jiraIssueKeyRegexp(projectKeys []string) *regexp.Regexp { + if len(projectKeys) == 0 { + return defaultJiraIssueKeyRegexp + } + return regexp.MustCompile(MakeJiraIssueKeyPattern(projectKeys)) +} + // FindJiraIssueKeys finds all Jira issue keys in text, filtering out // partial matches from multi-segment identifiers like CVE-2026-41284. // Matching is case-insensitive: the text is uppercased before the regex @@ -131,8 +151,7 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { // immediately followed by a hyphen and a digit. func FindJiraIssueKeys(text string, projectKeys []string) []string { upperText := strings.ToUpper(text) - pattern := MakeJiraIssueKeyPattern(projectKeys) - re := regexp.MustCompile(pattern) + re := jiraIssueKeyRegexp(projectKeys) candidates := re.FindAllString(upperText, -1) // Deduplicate (all candidates are already uppercase). @@ -146,10 +165,9 @@ func FindJiraIssueKeys(text string, projectKeys []string) []string { } // Filter out matches that are always followed by - in the uppercased text. - dashDigit := regexp.MustCompile(`^-\d`) var result []string for _, m := range unique { - if isPartialMultiSegment(upperText, m, dashDigit) { + if isPartialMultiSegment(upperText, m) { continue } result = append(result, m) @@ -166,7 +184,7 @@ func FindJiraIssueKeys(text string, projectKeys []string) []string { // is immediately followed by a "-" suffix, indicating it is part // of a longer multi-segment identifier (e.g. CVE-2026-41284). // Precondition: match must exist in text (guaranteed when called from FindJiraIssueKeys). -func isPartialMultiSegment(text, match string, dashDigit *regexp.Regexp) bool { +func isPartialMultiSegment(text, match string) bool { start := 0 for { idx := strings.Index(text[start:], match) @@ -174,7 +192,7 @@ func isPartialMultiSegment(text, match string, dashDigit *regexp.Regexp) bool { break } afterIdx := start + idx + len(match) - if afterIdx >= len(text) || !dashDigit.MatchString(text[afterIdx:]) { + if afterIdx >= len(text) || !dashDigitRegexp.MatchString(text[afterIdx:]) { return false } start = start + idx + 1 diff --git a/internal/jira/jira_test.go b/internal/jira/jira_test.go index 84acefa48..702436436 100644 --- a/internal/jira/jira_test.go +++ b/internal/jira/jira_test.go @@ -223,3 +223,17 @@ func TestFindJiraIssueKeys(t *testing.T) { }) } } + +func BenchmarkFindJiraIssueKeys(b *testing.B) { + text := "EX-1 fixes the regression reported in EX-2, see also branch bugfix/EX-3" + b.Run("default pattern", func(b *testing.B) { + for b.Loop() { + FindJiraIssueKeys(text, nil) + } + }) + b.Run("project key pattern", func(b *testing.B) { + for b.Loop() { + FindJiraIssueKeys(text, []string{"EX"}) + } + }) +} diff --git a/internal/kube/kube.go b/internal/kube/kube.go index 1fc7fe990..e2f8410d8 100644 --- a/internal/kube/kube.go +++ b/internal/kube/kube.go @@ -250,46 +250,17 @@ func (clientset *K8SConnection) filterNamespaces(filter *filters.ResourceFilterO return result, nil } - var ( - wg sync.WaitGroup - mutex = &sync.Mutex{} - ) - - errs := make(chan error, 1) // Buffered only for the first error - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() // Make sure it's called to release resources even if no errors + // compile the filter patterns once, instead of once per namespace + compiledFilter := filter.Compile() for _, ns := range nsList { - wg.Add(1) - go func(ns string) { - defer wg.Done() - - // Check if any error occurred in any other gorouties: - select { - case <-ctx.Done(): - return // Error somewhere, terminate - default: // Default is must to avoid blocking - } - - include, err := filter.ShouldInclude(ns) - if err != nil { - select { - case errs <- err: - default: - } - cancel() // send cancel signal to goroutines - return - } - if include { - mutex.Lock() - result = append(result, ns) - mutex.Unlock() - } - }(ns.Name) - } - wg.Wait() - if ctx.Err() != nil { - return result, <-errs + included, err := compiledFilter.ShouldInclude(ns.Name) + if err != nil { + return result, err + } + if included { + result = append(result, ns.Name) + } } return result, nil } diff --git a/internal/kube/kube_test.go b/internal/kube/kube_test.go index f7ff6a3d4..f9075d995 100644 --- a/internal/kube/kube_test.go +++ b/internal/kube/kube_test.go @@ -211,22 +211,20 @@ func (suite *KubeTestSuite) TestGetPodsDataWithThrottling() { func (suite *KubeTestSuite) TestFilterNamespaces() { type args struct { - nsList []corev1.Namespace - filter *filters.ResourceFilterOptions + namespaces []string + filter *filters.ResourceFilterOptions } for _, t := range []struct { - name string - args args - expectError bool - want []string + name string + args args + expectError bool + want []string + wantFiltered []string }{ { name: "invalid regex patterns return error", args: args{ - nsList: []corev1.Namespace{ - {ObjectMeta: metav1.ObjectMeta{Name: "ns1"}}, - {ObjectMeta: metav1.ObjectMeta{Name: "ns2"}}, - }, + namespaces: []string{"filter-err-a1", "filter-err-a2"}, filter: &filters.ResourceFilterOptions{ IncludeNamesRegex: []string{"["}, }, @@ -234,15 +232,51 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { expectError: true, want: []string{}, }, + { + name: "namespaces matching the include regex patterns are returned", + args: args{ + namespaces: []string{"filter-inc-a1", "filter-inc-a2", "filter-inc-b1"}, + filter: &filters.ResourceFilterOptions{ + IncludeNamesRegex: []string{"^filter-inc-a.*$"}, + }, + }, + want: []string{"filter-inc-a1", "filter-inc-a2"}, + }, + { + name: "namespaces matching the exclude regex patterns are filtered out", + args: args{ + namespaces: []string{"filter-exc-a1", "filter-exc-a2", "filter-exc-b1"}, + filter: &filters.ResourceFilterOptions{ + ExcludeNamesRegex: []string{"^filter-exc-a.*$"}, + }, + }, + wantFiltered: []string{"filter-exc-a1", "filter-exc-a2"}, + }, } { suite.Run(t.name, func() { + // namespace names must not be shared with another test method: AfterTest + // only asks for deletion, and a namespace lingers in Terminating for a + // while after that, so re-creating one by the same name fails with + // "object is being deleted". Hence the filter- prefix here. + for _, ns := range t.args.namespaces { + suite.createNamespace(ns) + } result, err := suite.clientset.filterNamespaces(t.args.filter) if t.expectError { require.Error(suite.T(), err, "error was expected but got none.") - } else { - require.NoErrorf(suite.T(), err, "error was NOT expected but got: %v.", err) - require.Equal(suite.T(), t.want, result, "TestFilterNamespaces: got %v -- want %v", result, t.want) + return + } + require.NoErrorf(suite.T(), err, "error was NOT expected but got: %v.", err) + if len(t.wantFiltered) > 0 { + require.Subset(suite.T(), result, []string{"filter-exc-b1"}, + "TestFilterNamespaces: %v should contain the non-excluded namespace", result) + for _, ns := range t.wantFiltered { + require.NotContains(suite.T(), result, ns, + "TestFilterNamespaces: %s should have been filtered out of %v", ns, result) + } + return } + require.ElementsMatch(suite.T(), t.want, result, "TestFilterNamespaces: got %v -- want %v", result, t.want) }) } From 54b4aa30dfd37e7d3f5166f51c73d40d913dc8c1 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 18:10:08 +0100 Subject: [PATCH 2/5] fix: address review of the regex compilation changes - filters: Compile clones the literal name slices, so the immutability that CompiledResourceFilter's doc comment promises - and that makes sharing one compiled filter across the ECS goroutines safe - holds for the literals and not just the patterns. Covered by a test that fails without the clone. - kube: filterNamespaces' four-way length check is exactly !filter.IsSet(), as getFilteredLambdaFuncs already spells it. Collapsing it also keeps ResourceFilterOptions.IsSet() in production use. - attest jira: validateJiraProjectKeys compiled a constant pattern on every invocation, with error handling for a failure that cannot happen. It is now a package-level jiraProjectKeyRegexp, which internal/jira's jiraIssueKeyRegexp doc comment can name instead of repeating the literal. - digest: the pattern constant is validSha256Pattern rather than validSha256regex, which was neither idiomatic Go nor distinguishable at a glance from the compiled validSha256Regexp beside it. - kube test: TestFilterNamespaces asserted with ElementsMatch, which cannot fail on ordering, while this change claims the returned order is deterministic. It now builds the expectation in the cluster's listing order and asserts equality, with a guard so that assertion cannot pass vacuously. Also adds the k8s case for an invalid pattern behind an excluded literal name: the snapshot still fails there, because the cluster's own namespaces are not in ExcludeNames and one of them always reaches the pattern. Refs #826 --- cmd/kosli/attestJira.go | 21 ++++++++------- internal/digest/digest.go | 6 ++--- internal/filters/resourceFilter.go | 7 +++-- internal/filters/resourceFilter_test.go | 19 ++++++++++++++ internal/jira/jira.go | 4 +-- internal/kube/kube.go | 3 +-- internal/kube/kube_test.go | 34 ++++++++++++++++++++++++- 7 files changed, 75 insertions(+), 19 deletions(-) diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index 96289bfa9..8465e5952 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -380,18 +380,21 @@ func jiraSearchText(commitInfo *gitview.CommitInfo, secondarySource string, igno return strings.Join(searchTexts, "\n") } -func (o *attestJiraOptions) validateJiraProjectKeys() error { - // According to Jira documentation https://confluence.atlassian.com/adminjiraserver/changing-the-project-key-format-938847081.html - // the Jira project key has to start with a capital letter and can then have capital letters numbers and underscore. - // But Jira itself will accept lower case letters when searching a repository for matching branches and commits - matchesJiraProjectKeys, err := regexp.Compile("^[A-Za-z][A-Za-z0-9_]{1,9}$") - if err != nil { - return err - } +// jiraProjectKeyRegexp is compiled once, so validateJiraProjectKeys does not re-compile a +// constant pattern on every invocation. +// +// According to Jira documentation https://confluence.atlassian.com/adminjiraserver/changing-the-project-key-format-938847081.html +// the Jira project key has to start with a capital letter and can then have capital letters numbers and underscore. +// But Jira itself will accept lower case letters when searching a repository for matching branches and commits. +// +// jira.MakeJiraIssueKeyPattern relies on this: a key that matches here carries no regex +// metacharacters, so building a pattern from validated keys cannot fail to compile. +var jiraProjectKeyRegexp = regexp.MustCompile("^[A-Za-z][A-Za-z0-9_]{1,9}$") +func (o *attestJiraOptions) validateJiraProjectKeys() error { invalidKeys := []string{} for _, projectKey := range o.projectKeys { - isValid := matchesJiraProjectKeys.MatchString(projectKey) + isValid := jiraProjectKeyRegexp.MatchString(projectKey) if !isValid { invalidKeys = append(invalidKeys, projectKey) } diff --git a/internal/digest/digest.go b/internal/digest/digest.go index ab09b942a..2eea9efe7 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -343,16 +343,16 @@ func RemoteDockerImageSha256(client *requests.Client, imageName, imageTag, regis return strings.TrimPrefix(digestHeader, "sha256:"), nil } -const validSha256regex = "^([a-f0-9]{64})$" +const validSha256Pattern = "^([a-f0-9]{64})$" // validSha256Regexp is compiled once, so ValidateDigest does not re-compile a // constant pattern on every call. -var validSha256Regexp = regexp.MustCompile(validSha256regex) +var validSha256Regexp = regexp.MustCompile(validSha256Pattern) // ValidateDigest checks if a digest matches the sha256 regex func ValidateDigest(sha256ToCheck string) error { if !validSha256Regexp.MatchString(sha256ToCheck) { - return fmt.Errorf("%s is not a valid SHA256 fingerprint. It should match the pattern %v", sha256ToCheck, validSha256regex) + return fmt.Errorf("%s is not a valid SHA256 fingerprint. It should match the pattern %v", sha256ToCheck, validSha256Pattern) } return nil } diff --git a/internal/filters/resourceFilter.go b/internal/filters/resourceFilter.go index 829eecd58..53052eec0 100644 --- a/internal/filters/resourceFilter.go +++ b/internal/filters/resourceFilter.go @@ -63,11 +63,14 @@ type CompiledResourceFilter struct { // reported by ShouldInclude when matching a name reaches that pattern. Reporting it here // instead would reject filters that never reach it, such as a name settled by // ExcludeNames, or an invalid pattern behind one that already matched. +// +// The literal name slices are cloned, so that mutating the options afterwards cannot +// change what an already-compiled filter matches. func (filter *ResourceFilterOptions) Compile() *CompiledResourceFilter { return &CompiledResourceFilter{ - includeNames: filter.IncludeNames, + includeNames: slices.Clone(filter.IncludeNames), includePatterns: compileNamesRegex(filter.IncludeNamesRegex, "include"), - excludeNames: filter.ExcludeNames, + excludeNames: slices.Clone(filter.ExcludeNames), excludePatterns: compileNamesRegex(filter.ExcludeNamesRegex, "exclude"), } } diff --git a/internal/filters/resourceFilter_test.go b/internal/filters/resourceFilter_test.go index fedb3224c..f77de4713 100644 --- a/internal/filters/resourceFilter_test.go +++ b/internal/filters/resourceFilter_test.go @@ -262,6 +262,25 @@ func (suite *FiltersSuite) TestCompile() { } } +// TestCompileClonesLiteralNames pins the immutability CompiledResourceFilter's doc comment +// claims, and which makes it safe to share one compiled filter across goroutines: mutating +// the options after Compile must not change what the compiled filter matches. +func (suite *FiltersSuite) TestCompileClonesLiteralNames() { + filter := &ResourceFilterOptions{ExcludeNames: []string{"foo"}} + compiled := filter.Compile() + + filter.ExcludeNames[0] = "bar" + filter.ExcludeNames = append(filter.ExcludeNames, "baz") + + included, err := compiled.ShouldInclude("foo") + require.NoError(suite.T(), err) + require.False(suite.T(), included, "foo was excluded when the filter was compiled") + + included, err = compiled.ShouldInclude("bar") + require.NoError(suite.T(), err) + require.True(suite.T(), included, "bar was not excluded when the filter was compiled") +} + // TestCompiledFilterConcurrentShouldInclude verifies that one compiled filter can be // shared by many goroutines, as the k8s and ECS snapshot paths do. The filter carries an // invalid pattern so that the error path is exercised concurrently too. Run with -race. diff --git a/internal/jira/jira.go b/internal/jira/jira.go index af5a6a7fb..90a13a436 100644 --- a/internal/jira/jira.go +++ b/internal/jira/jira.go @@ -134,8 +134,8 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { // jiraIssueKeyRegexp returns the compiled issue key pattern for the given project keys. // The default pattern is compiled once; a project-key pattern is compiled per call, which -// is safe from panics because validateJiraProjectKeys has already rejected any key outside -// ^[A-Za-z][A-Za-z0-9_]{1,9}$, so a key cannot carry regex metacharacters. +// is safe from panics because validateJiraProjectKeys has already rejected any key that +// jiraProjectKeyRegexp does not match, so a key cannot carry regex metacharacters. func jiraIssueKeyRegexp(projectKeys []string) *regexp.Regexp { if len(projectKeys) == 0 { return defaultJiraIssueKeyRegexp diff --git a/internal/kube/kube.go b/internal/kube/kube.go index e2f8410d8..9ccd06851 100644 --- a/internal/kube/kube.go +++ b/internal/kube/kube.go @@ -242,8 +242,7 @@ func (clientset *K8SConnection) filterNamespaces(filter *filters.ResourceFilterO return result, err } - if len(filter.IncludeNames) == 0 && len(filter.IncludeNamesRegex) == 0 && - len(filter.ExcludeNames) == 0 && len(filter.ExcludeNamesRegex) == 0 { + if !filter.IsSet() { for _, ns := range nsList { result = append(result, ns.Name) } diff --git a/internal/kube/kube_test.go b/internal/kube/kube_test.go index f9075d995..049ce1cf5 100644 --- a/internal/kube/kube_test.go +++ b/internal/kube/kube_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "testing" "time" @@ -252,6 +253,23 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { }, wantFiltered: []string{"filter-exc-a1", "filter-exc-a2"}, }, + { + // An invalid pattern is only reported once matching a name reaches it, and a + // name listed in ExcludeNames is settled before that. On this path the + // short-circuit never saves the snapshot though: the cluster's own + // namespaces (default, kube-system, ...) are not in ExcludeNames, so one of + // them always reaches the pattern. + name: "an invalid exclude pattern is reported despite the excluded literal name", + args: args{ + namespaces: []string{"filter-lazy-a1"}, + filter: &filters.ResourceFilterOptions{ + ExcludeNames: []string{"filter-lazy-a1"}, + ExcludeNamesRegex: []string{"["}, + }, + }, + expectError: true, + want: []string{}, + }, } { suite.Run(t.name, func() { // namespace names must not be shared with another test method: AfterTest @@ -276,7 +294,21 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { } return } - require.ElementsMatch(suite.T(), t.want, result, "TestFilterNamespaces: got %v -- want %v", result, t.want) + // filterNamespaces returns the namespaces in the order the cluster listed + // them, which the goroutine-per-namespace fan-out it replaced could not + // guarantee. Build the expectation in that same order so the assertion + // pins the ordering and not just the set. + nsList, err := suite.clientset.GetClusterNamespaces() + require.NoErrorf(suite.T(), err, "error listing cluster namespaces") + wantInClusterOrder := []string{} + for _, ns := range nsList { + if slices.Contains(t.want, ns.Name) { + wantInClusterOrder = append(wantInClusterOrder, ns.Name) + } + } + require.ElementsMatch(suite.T(), t.want, wantInClusterOrder, + "TestFilterNamespaces: %v missing from the cluster listing, the ordering assertion would be vacuous", t.want) + require.Equal(suite.T(), wantInClusterOrder, result, "TestFilterNamespaces: got %v -- want %v", result, wantInClusterOrder) }) } From 557195701c4191cc71bf03e11b3220b6a3e97c05 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 18:10:10 +0100 Subject: [PATCH 3/5] perf: compile the annotation key pattern once processAnnotations re-compiled a constant pattern for every annotation key, which is the same shape as the digest and jira changes except that this one is inside the loop rather than once per invocation. Hoist it to a package-level annotationKeyRegexp, matching jiraProjectKeyRegexp. The pattern and the error message are unchanged, so the existing --annotate cases still pin the behaviour. This is the last constant pattern compiled inline outside tests; the remaining regexp.Compile calls all take a user-supplied pattern and have to stay dynamic. Also clone the namespace list in filterNamespaces' early return, which handed back the caller's slice fifteen lines above a Compile that clones precisely so callers cannot alias. Nothing mutates it today, so this is for consistency rather than a fix. Refs #826 --- cmd/kosli/attestation.go | 6 +++++- internal/kube/kube.go | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/cmd/kosli/attestation.go b/cmd/kosli/attestation.go index 22a1d45ee..e0d747156 100644 --- a/cmd/kosli/attestation.go +++ b/cmd/kosli/attestation.go @@ -180,9 +180,13 @@ func validateRepoFlags(repoURL, repoProvider string, validateURL bool) error { return nil } +// annotationKeyRegexp is compiled once, so processAnnotations does not re-compile a +// constant pattern for every annotation key it checks. +var annotationKeyRegexp = regexp.MustCompile(`^[A-Za-z0-9_]+$`) + func processAnnotations(annotations map[string]string) (map[string]string, error) { for label := range annotations { - if !regexp.MustCompile(`^[A-Za-z0-9_]+$`).MatchString(label) { + if !annotationKeyRegexp.MatchString(label) { return nil, fmt.Errorf("--annotate flag should be in the format key=value. Invalid key: '%s'. Key can only contain [A-Za-z0-9_]", label) } } diff --git a/internal/kube/kube.go b/internal/kube/kube.go index 9ccd06851..32517853b 100644 --- a/internal/kube/kube.go +++ b/internal/kube/kube.go @@ -3,6 +3,7 @@ package kube import ( "context" "fmt" + "slices" "sync" "github.com/kosli-dev/cli/internal/filters" @@ -232,7 +233,9 @@ func processPods(list *corev1.PodList, logger *logger.Logger) ([]*PodData, error func (clientset *K8SConnection) filterNamespaces(filter *filters.ResourceFilterOptions) ([]string, error) { if len(filter.IncludeNamesRegex) == 0 && len(filter.ExcludeNamesRegex) == 0 { if len(filter.IncludeNames) > 0 { - return filter.IncludeNames, nil + // cloned for the same reason Compile clones them: the caller keeps the + // options, and the result must not alias a slice they can still change + return slices.Clone(filter.IncludeNames), nil } } result := []string{} From d583bc982caa7ccf368836c2999343e2911934b6 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 18:10:11 +0100 Subject: [PATCH 4/5] refactor(jira): quote project keys when building the issue key pattern MakeJiraIssueKeyPattern interpolated project keys into a regex verbatim, so "the pattern always compiles" held only because validateJiraProjectKeys had already rejected anything containing a metacharacter. That contract lived in a comment in cmd/kosli and was relied on by a MustCompile in internal/jira, two packages away from the check enforcing it. QuoteMeta each key instead, which makes it a property of the function rather than a promise its callers have to keep - and lets MakeJiraIssueKeyPattern, which is exported, be safe on its own terms. No behaviour change for a key a Jira project could actually have: none of [A-Za-z0-9_] is a metacharacter, so quoting leaves every valid key untouched and the existing pattern expectations are unchanged. Adds a case for keys that do carry metacharacters, which fails without the quoting. Refs #826 --- cmd/kosli/attestJira.go | 3 --- internal/jira/jira.go | 10 ++++++---- internal/jira/jira_test.go | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index 8465e5952..c0c10f19e 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -386,9 +386,6 @@ func jiraSearchText(commitInfo *gitview.CommitInfo, secondarySource string, igno // According to Jira documentation https://confluence.atlassian.com/adminjiraserver/changing-the-project-key-format-938847081.html // the Jira project key has to start with a capital letter and can then have capital letters numbers and underscore. // But Jira itself will accept lower case letters when searching a repository for matching branches and commits. -// -// jira.MakeJiraIssueKeyPattern relies on this: a key that matches here carries no regex -// metacharacters, so building a pattern from validated keys cannot fail to compile. var jiraProjectKeyRegexp = regexp.MustCompile("^[A-Za-z][A-Za-z0-9_]{1,9}$") func (o *attestJiraOptions) validateJiraProjectKeys() error { diff --git a/internal/jira/jira.go b/internal/jira/jira.go index 90a13a436..e515ac7f8 100644 --- a/internal/jira/jira.go +++ b/internal/jira/jira.go @@ -125,17 +125,19 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { if len(projectKeys) == 0 { return defaultJiraIssueKeyPattern } + // each key is quoted, so the returned pattern always compiles whatever the caller + // passes. A key that a Jira project could actually have carries no regex + // metacharacters, so quoting leaves it unchanged. upper := make([]string, len(projectKeys)) for i, k := range projectKeys { - upper[i] = strings.ToUpper(k) + upper[i] = regexp.QuoteMeta(strings.ToUpper(k)) } return `\b(` + strings.Join(upper, "|") + `)-[0-9]+` } // jiraIssueKeyRegexp returns the compiled issue key pattern for the given project keys. -// The default pattern is compiled once; a project-key pattern is compiled per call, which -// is safe from panics because validateJiraProjectKeys has already rejected any key that -// jiraProjectKeyRegexp does not match, so a key cannot carry regex metacharacters. +// The default pattern is compiled once; a project-key pattern is compiled per call, and +// cannot panic because MakeJiraIssueKeyPattern quotes the keys it interpolates. func jiraIssueKeyRegexp(projectKeys []string) *regexp.Regexp { if len(projectKeys) == 0 { return defaultJiraIssueKeyRegexp diff --git a/internal/jira/jira_test.go b/internal/jira/jira_test.go index 702436436..2c4506f02 100644 --- a/internal/jira/jira_test.go +++ b/internal/jira/jira_test.go @@ -61,6 +61,23 @@ func TestMakeJiraIssueKey(t *testing.T) { "DEF-123", }, }, + { + // A real Jira project key cannot contain these, and validateJiraProjectKeys + // rejects them before they reach here, but this is an exported function: + // quoting the keys is what makes "the pattern always compiles" a property of + // the code rather than a promise the caller has to keep. + name: "Project keys carrying regex metacharacters are quoted", + projectKeys: []string{"a(b", "c[d"}, + want: `\b(A\(B|C\[D)-[0-9]+`, + matches: []string{ + "A(B-123", + "C[D-456", + }, + nonMatches: []string{ + "AB-123", + "AXB-123", + }, + }, } for _, tt := range tests { From a032f6f6110ce4eac813055862527efab9e2a7e3 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 18:10:12 +0100 Subject: [PATCH 5/5] fix(jira): narrow rather than widen on unusable project keys Follow-up to the quoting change, addressing review of it. makeJiraIssueKeyPattern is now unexported. Its "" return means "no issue key can match", but it is the one sentinel that inverts when used: MustCompile("") succeeds and matches at every position, so a caller who compiles it gets match-everything, the widening the sentinel exists to prevent. Only jiraIssueKeyRegexp stood between the two, and the function had just two references, both in this package. Its rationale also sat inside the function body, so go doc printed a bare signature for a return value with three meanings; that is now a doc comment listing all three and saying the empty one must not be compiled. Keys that all drop out no longer fall back to the default pattern. Asking for no project in particular and asking for projects that all turn out to be unusable are different questions: the first means every project, the second means none can match. Answering the second with the default would widen a caller who named projects to every project, and on an attestation path the keys that widening invents are then looked up and attested. Blank keys are dropped and the rest trimmed. An empty key interpolates an empty alternative that matches any -[0-9]+, so {"EX", ""} reported -41284 out of CVE-2026-41284 as an issue key; a whitespace-only key does the same one column over, since a space is not a metacharacter for QuoteMeta to escape; and an untrimmed " EX" reports " EX-12", which is not a key any project has. None of this is reachable from the CLI - validateJiraProjectKeys rejects all three before FindJiraIssueKeys sees them - so the guard is kept to one line and one test case rather than covered exhaustively. Also in internal/kube's TestFilterNamespaces: drop the namespaces from the invalid-pattern case, which created and deleted two against a real cluster without needing them, since the cluster's own namespaces reach the pattern. The other error case keeps its namespace, which is load-bearing - it exists to be settled by ExcludeNames while the others still reach the invalid pattern. And state plainly what TestCompiledFilterConcurrentShouldInclude does establish: every goroutine writes its own slot and reads only immutable state, so its assertions check that concurrent matching returns the right answers, not that the sharing is synchronised. Refs #826 --- internal/filters/resourceFilter_test.go | 10 +++- internal/jira/jira.go | 74 +++++++++++++++++++------ internal/jira/jira_test.go | 50 +++++++++++++++-- internal/kube/kube_test.go | 20 +++++-- 4 files changed, 124 insertions(+), 30 deletions(-) diff --git a/internal/filters/resourceFilter_test.go b/internal/filters/resourceFilter_test.go index f77de4713..5e458b9d1 100644 --- a/internal/filters/resourceFilter_test.go +++ b/internal/filters/resourceFilter_test.go @@ -281,9 +281,13 @@ func (suite *FiltersSuite) TestCompileClonesLiteralNames() { require.True(suite.T(), included, "bar was not excluded when the filter was compiled") } -// TestCompiledFilterConcurrentShouldInclude verifies that one compiled filter can be -// shared by many goroutines, as the k8s and ECS snapshot paths do. The filter carries an -// invalid pattern so that the error path is exercised concurrently too. Run with -race. +// TestCompiledFilterConcurrentShouldInclude shares one compiled filter across many +// goroutines, as the k8s and ECS snapshot paths do. The filter carries an invalid pattern +// so that the error path is exercised concurrently too. +// +// Every goroutine writes its own slot and reads only immutable state, so the assertions +// check that concurrent matching returns the right answers - both of them, for the same +// filter - rather than that the sharing itself is synchronised. func (suite *FiltersSuite) TestCompiledFilterConcurrentShouldInclude() { filter := &ResourceFilterOptions{ IncludeNamesRegex: []string{"^include-.*$", "^never-reached["}, diff --git a/internal/jira/jira.go b/internal/jira/jira.go index e515ac7f8..902db4f8f 100644 --- a/internal/jira/jira.go +++ b/internal/jira/jira.go @@ -116,33 +116,67 @@ var ( dashDigitRegexp = regexp.MustCompile(`^-\d`) ) -func MakeJiraIssueKeyPattern(projectKeys []string) string { - // Jira issue keys consist of [project-key]-[sequential-number]. - // FindJiraIssueKeys uppercases the text before applying this pattern, so the - // pattern only needs to handle uppercase. Project keys supplied by the caller - // are also uppercased here for the same reason. - // more info: https://support.atlassian.com/jira-software-cloud/docs/what-is-an-issue/#Workingwithissues-Projectandissuekeys +// makeJiraIssueKeyPattern builds the regex matching Jira issue keys of the given projects. +// Jira issue keys consist of [project-key]-[sequential-number]; see +// https://support.atlassian.com/jira-software-cloud/docs/what-is-an-issue/#Workingwithissues-Projectandissuekeys +// +// The return value carries three distinct meanings: +// +// - no project keys at all: the default pattern, matching keys of every project. +// - at least one usable key: a pattern matching keys of those projects only. +// - keys given, none of them usable: "", meaning no issue key can match. +// +// The "" case must NOT be compiled directly - the empty pattern matches at every position, +// so compiling it yields the exact opposite of what it means. Use jiraIssueKeyRegexp, which +// maps it to a nil regexp; this function is unexported so that nothing else can get it +// wrong. It is separate from the no-keys case on purpose: answering "these projects all +// turned out to be unusable" with the default pattern would widen a caller who named +// projects to every project, and on an attestation path the keys that widening invents are +// then looked up and attested. +// +// FindJiraIssueKeys uppercases the text before applying the pattern, so the pattern only +// needs to handle uppercase, and the project keys are uppercased here for the same reason. +// Each key is also quoted, so the pattern always compiles whatever the caller passes; a key +// a Jira project could actually have carries no regex metacharacters, so quoting leaves it +// unchanged. Keys are trimmed, and blank ones dropped rather than interpolated: an empty +// alternative reduces the group to nothing and leaves a pattern matching any -[0-9]+, which +// reports a phantom key such as -41284 out of CVE-2026-41284, and a whitespace-only key does +// the same thing one column over, since a space is not a metacharacter for QuoteMeta to +// escape. Trimming also stops " PROJ" from reporting " PROJ-123", which is not a key any +// Jira project has. +func makeJiraIssueKeyPattern(projectKeys []string) string { if len(projectKeys) == 0 { return defaultJiraIssueKeyPattern } - // each key is quoted, so the returned pattern always compiles whatever the caller - // passes. A key that a Jira project could actually have carries no regex - // metacharacters, so quoting leaves it unchanged. - upper := make([]string, len(projectKeys)) - for i, k := range projectKeys { - upper[i] = regexp.QuoteMeta(strings.ToUpper(k)) + upper := make([]string, 0, len(projectKeys)) + for _, k := range projectKeys { + k = strings.TrimSpace(k) + if k == "" { + continue + } + upper = append(upper, regexp.QuoteMeta(strings.ToUpper(k))) + } + if len(upper) == 0 { + return "" } return `\b(` + strings.Join(upper, "|") + `)-[0-9]+` } -// jiraIssueKeyRegexp returns the compiled issue key pattern for the given project keys. -// The default pattern is compiled once; a project-key pattern is compiled per call, and -// cannot panic because MakeJiraIssueKeyPattern quotes the keys it interpolates. +// jiraIssueKeyRegexp returns the compiled issue key pattern for the given project keys, or +// nil if no issue key can match, which makeJiraIssueKeyPattern reports as "". +// +// A project-key pattern is compiled per call, and cannot panic because +// makeJiraIssueKeyPattern quotes the keys it interpolates. When the pattern is the default +// one, the copy compiled at package level is returned instead. func jiraIssueKeyRegexp(projectKeys []string) *regexp.Regexp { - if len(projectKeys) == 0 { + switch pattern := makeJiraIssueKeyPattern(projectKeys); pattern { + case "": + return nil + case defaultJiraIssueKeyPattern: return defaultJiraIssueKeyRegexp + default: + return regexp.MustCompile(pattern) } - return regexp.MustCompile(MakeJiraIssueKeyPattern(projectKeys)) } // FindJiraIssueKeys finds all Jira issue keys in text, filtering out @@ -152,8 +186,12 @@ func jiraIssueKeyRegexp(projectKeys []string) *regexp.Regexp { // A match is discarded if every occurrence in the uppercased text is // immediately followed by a hyphen and a digit. func FindJiraIssueKeys(text string, projectKeys []string) []string { - upperText := strings.ToUpper(text) re := jiraIssueKeyRegexp(projectKeys) + if re == nil { + // project keys were given but none of them is usable, so no key can belong to them + return nil + } + upperText := strings.ToUpper(text) candidates := re.FindAllString(upperText, -1) // Deduplicate (all candidates are already uppercase). diff --git a/internal/jira/jira_test.go b/internal/jira/jira_test.go index 2c4506f02..67498f7fa 100644 --- a/internal/jira/jira_test.go +++ b/internal/jira/jira_test.go @@ -63,9 +63,9 @@ func TestMakeJiraIssueKey(t *testing.T) { }, { // A real Jira project key cannot contain these, and validateJiraProjectKeys - // rejects them before they reach here, but this is an exported function: - // quoting the keys is what makes "the pattern always compiles" a property of - // the code rather than a promise the caller has to keep. + // rejects them before they reach here: quoting is what makes "the pattern + // always compiles" a property of the code rather than a promise its callers + // have to keep. name: "Project keys carrying regex metacharacters are quoted", projectKeys: []string{"a(b", "c[d"}, want: `\b(A\(B|C\[D)-[0-9]+`, @@ -78,11 +78,37 @@ func TestMakeJiraIssueKey(t *testing.T) { "AXB-123", }, }, + { + // Keys are trimmed and blank ones dropped, so that --jira-project-key "EX, " + // cannot widen the pattern: an empty alternative matches any -[0-9]+, a + // whitespace-only key does the same one column over, and an untrimmed " EX" + // would report " EX-12" as the key. + name: "Blank project keys are dropped and the rest trimmed", + projectKeys: []string{" EX ", "", " ", "\t"}, + want: `\b(EX)-[0-9]+`, + matches: []string{ + "EX-12", + }, + nonMatches: []string{ + "CVE-2026-41284", + "-41284", + "fix -123", + "XEX-12", + }, + }, + { + // Not the default pattern: the caller named projects, so widening to every + // project would answer a question they did not ask. "" means nothing can + // match, which FindJiraIssueKeys turns into no keys. + name: "Project keys that all drop out match nothing rather than everything", + projectKeys: []string{"", " ", "\t"}, + want: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := MakeJiraIssueKeyPattern(tt.projectKeys) + got := makeJiraIssueKeyPattern(tt.projectKeys) if got != tt.want { t.Errorf("makeJiraIssueKeyPattern() = %v, want %v", got, tt.want) } @@ -132,6 +158,22 @@ func TestFindJiraIssueKeys(t *testing.T) { projectKeys: []string{}, want: nil, }, + { + // The empty key must not widen the pattern to any -[0-9]+, which would + // report -41284 out of the CVE as a key of project EX. + name: "an empty project key alongside a real one finds only the real project's keys", + text: "EX-12 fixes CVE-2026-41284", + projectKeys: []string{"EX", ""}, + want: []string{"EX-12"}, + }, + { + // And project keys that all drop out find nothing, rather than falling back + // to every project and returning PROJ-42. + name: "project keys that all drop out find nothing", + text: "PROJ-42 fixes CVE-2026-41284", + projectKeys: []string{"", ""}, + want: nil, + }, { name: "multiple CVE identifiers produce no matches", text: "CVE-2026-41284 and CVE-2025-12345", diff --git a/internal/kube/kube_test.go b/internal/kube/kube_test.go index 049ce1cf5..102dd5cef 100644 --- a/internal/kube/kube_test.go +++ b/internal/kube/kube_test.go @@ -223,15 +223,17 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { wantFiltered []string }{ { + // Creates no namespaces: the invalid pattern is reported as soon as filtering + // reaches it, and the cluster's own namespaces (default, kube-system, ...) + // are enough to reach it. Any created here would be a create and a delete + // against a real cluster for nothing. name: "invalid regex patterns return error", args: args{ - namespaces: []string{"filter-err-a1", "filter-err-a2"}, filter: &filters.ResourceFilterOptions{ IncludeNamesRegex: []string{"["}, }, }, expectError: true, - want: []string{}, }, { name: "namespaces matching the include regex patterns are returned", @@ -268,7 +270,6 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { }, }, expectError: true, - want: []string{}, }, } { suite.Run(t.name, func() { @@ -286,8 +287,17 @@ func (suite *KubeTestSuite) TestFilterNamespaces() { } require.NoErrorf(suite.T(), err, "error was NOT expected but got: %v.", err) if len(t.wantFiltered) > 0 { - require.Subset(suite.T(), result, []string{"filter-exc-b1"}, - "TestFilterNamespaces: %v should contain the non-excluded namespace", result) + // every namespace this case created and did not ask to be filtered out + // has to survive, so the case describes its own expectation instead of + // naming a survivor twice + survivors := []string{} + for _, ns := range t.args.namespaces { + if !slices.Contains(t.wantFiltered, ns) { + survivors = append(survivors, ns) + } + } + require.Subset(suite.T(), result, survivors, + "TestFilterNamespaces: %v should contain every non-excluded namespace %v", result, survivors) for _, ns := range t.wantFiltered { require.NotContains(suite.T(), result, ns, "TestFilterNamespaces: %s should have been filtered out of %v", ns, result)