diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index 8b22b9760..c0c10f19e 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,18 +366,32 @@ func (o *attestJiraOptions) run(args []string) error { return wrapAttestationError(err) } -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 +// 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") +} + +// 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. +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/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/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/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..2eea9efe7 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -343,15 +343,16 @@ func RemoteDockerImageSha256(client *requests.Client, imageName, imageTag, regis return strings.TrimPrefix(digestHeader, "sha256:"), nil } +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(validSha256Pattern) + // 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) { - return fmt.Errorf("%s is not a valid SHA256 fingerprint. It should match the pattern %v", sha256ToCheck, validSha256regex) + if !validSha256Regexp.MatchString(sha256ToCheck) { + return fmt.Errorf("%s is not a valid SHA256 fingerprint. It should match the pattern %v", sha256ToCheck, validSha256Pattern) } 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..53052eec0 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,113 @@ 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. +// +// 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: slices.Clone(filter.IncludeNames), + includePatterns: compileNamesRegex(filter.IncludeNamesRegex, "include"), + excludeNames: slices.Clone(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..5e458b9d1 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,264 @@ 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.Equal(suite.T(), answer, t.want) + 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.False(suite.T(), included, "expected %s to NOT be included", name) } }) } } +// 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 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["}, + } + 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..902db4f8f 100644 --- a/internal/jira/jira.go +++ b/internal/jira/jira.go @@ -107,22 +107,78 @@ func (jc *JiraConfig) GetJiraIssueInfo(issueID string, issueFields string) (*Jir return result, nil } -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 +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`) +) + +// 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 `\b[A-Z][A-Z0-9]{1,9}-[0-9]+` + return defaultJiraIssueKeyPattern + } + 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))) } - upper := make([]string, len(projectKeys)) - for i, k := range projectKeys { - upper[i] = 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, 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 { + switch pattern := makeJiraIssueKeyPattern(projectKeys); pattern { + case "": + return nil + case defaultJiraIssueKeyPattern: + return defaultJiraIssueKeyRegexp + default: + return regexp.MustCompile(pattern) + } +} + // 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 @@ -130,9 +186,12 @@ func MakeJiraIssueKeyPattern(projectKeys []string) string { // 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 { + 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) - pattern := MakeJiraIssueKeyPattern(projectKeys) - re := regexp.MustCompile(pattern) candidates := re.FindAllString(upperText, -1) // Deduplicate (all candidates are already uppercase). @@ -146,10 +205,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 +224,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 +232,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..67498f7fa 100644 --- a/internal/jira/jira_test.go +++ b/internal/jira/jira_test.go @@ -61,11 +61,54 @@ func TestMakeJiraIssueKey(t *testing.T) { "DEF-123", }, }, + { + // A real Jira project key cannot contain these, and validateJiraProjectKeys + // 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]+`, + matches: []string{ + "A(B-123", + "C[D-456", + }, + nonMatches: []string{ + "AB-123", + "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) } @@ -115,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", @@ -223,3 +282,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..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{} @@ -242,54 +245,24 @@ 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) } 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..102dd5cef 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" @@ -211,38 +212,113 @@ 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 }{ { + // 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{ - nsList: []corev1.Namespace{ - {ObjectMeta: metav1.ObjectMeta{Name: "ns1"}}, - {ObjectMeta: metav1.ObjectMeta{Name: "ns2"}}, - }, filter: &filters.ResourceFilterOptions{ IncludeNamesRegex: []string{"["}, }, }, 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"}, + }, + { + // 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, }, } { 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 { + // 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) + } + return + } + // 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) }) }