Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(securitycenter): issues related to flaky tests in SHA & ETD #5090

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

vijaykanthm
Copy link
Contributor

@vijaykanthm vijaykanthm commented Jan 16, 2025

Description

Fixes #4866, #4871 Flakiness in tests due to Concurrent Modification error by refactoring the clean up process of created modules for testing.

This pull request addresses flaky tests in the golang-samples repository, specifically those related to Security Health Analytics (SHA) and Event Threat Detection (ETD). The core issue was a concurrent modification error causing test instability.

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed Contributing Guidelines from CONTRIBUTING.MD
  • Tests pass: go test -v ./.. (see Testing)
  • Code formatted: gofmt (see Formatting)
  • Vetting pass: go vet (see Formatting)
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@vijaykanthm vijaykanthm requested review from a team as code owners January 16, 2025 00:04
@product-auto-label product-auto-label bot added api: securitycenter Issues related to the Security Command Center API. samples Issues that are directly related to samples. labels Jan 16, 2025
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, here's a summary of the changes for you and other reviewers to quickly get up to speed:

This pull request, authored by vijaykanthm, aims to fix flaky tests in the golang-samples repository related to Security Health Analytics (SHA) and Event Threat Detection (ETD). The root cause was identified as a concurrent modification error.

Here's a breakdown of the changes:

  • Improved Uniqueness in Custom Module Creation: The code now uses the google/uuid package to generate unique suffixes for custom module display names. This eliminates the previous reliance on math/rand and time.Now(), which could lead to collisions and flaky tests. Specifically:

    • In securitycenter/management_api/create_security_health_analytics_custom_module.go, lines 45-49 were modified to use UUIDs and a regular expression to sanitize the generated suffix for valid characters. The old random number generation was removed (lines 23, 24).
    • Similar changes were made in securitycenter/management_api/event_threat_detection/create_event_threat_detection_custom_module.go, lines 41-44 were updated to use UUIDs. The old random number generation was removed (lines 23, 24).
    • The test file securitycenter/management_api/event_threat_detection/event_threat_detection_custom_module_test.go was significantly refactored to improve test stability and cleanup. The old random number generation was removed (lines 22, 26). New functions were added to manage cleanup of created modules: AddModuleToCleanup, PrintAllCreatedModules, CleanupCreatedModules, CustomModuleExists, and CleanupAfterTests. The cleanupCustomModule and cleanupExistingCustomModules functions were removed.
    • Similar changes were made in securitycenter/management_api/security_health_analytics_custom_module_test.go, lines 22-34 were modified to use UUIDs and a regular expression to sanitize the generated suffix for valid characters. The old random number generation was removed (lines 22, 26). New functions were added to manage cleanup of created modules: AddModuleToCleanup, PrintAllCreatedModules, CleanupCreatedModules, CustomModuleExists, and CleanupAfterTests. The cleanupCustomModule and cleanupExistingCustomModules functions were removed.
  • Enhanced Test Stability: The test suite now includes robust cleanup mechanisms to ensure that test runs don't interfere with each other. This addresses the flakiness caused by concurrent modifications.

  • Improved Error Handling: The code now includes more comprehensive error handling to improve the overall robustness of the application.

The pull request addresses issues #4866 and #4871.

I'll post my detailed review shortly.

Meanwhile, here's a little poem for your enjoyment:

Tests once flaky, now run true,
UUIDs banish errors new.
Clean code sings a joyful tune,
Review complete, by afternoon!

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This PR introduces a good fix for the flaky tests by using UUIDs and cleaning up the created custom modules after tests. The implementation is well-structured and addresses the concurrency issues effectively. However, there are a few minor improvements that can be made.

Comment on lines +47 to +49
// Remove invalid characters (anything that isn't alphanumeric or an underscore)
re := regexp.MustCompile(`[^a-zA-Z0-9_]`)
uniqueSuffix = re.ReplaceAllString(uniqueSuffix, "_")

Choose a reason for hiding this comment

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

low

It's a good practice to precompile the regex for better performance.

47:	// Remove invalid characters (anything that isn't alphanumeric or an underscore)
48:	var re = regexp.MustCompile(`[^a-zA-Z0-9_]`)
49:	uniqueSuffix = re.ReplaceAllString(uniqueSuffix, "_")

rand.Seed(time.Now().UnixNano())
// Generate a unique suffix
uniqueSuffix := fmt.Sprintf("%d-%d", time.Now().Unix(), rand.Intn(1000))
uniqueSuffix := uuid.New().String()

Choose a reason for hiding this comment

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

low

Consider adding logic to handle potential UUID collisions, similar to what's done in create_security_health_analytics_custom_module.go.

44:	uniqueSuffix := uuid.New().String()
45:
46:	// Remove invalid characters (anything that isn't alphanumeric or an underscore)
47:	re := regexp.MustCompile(`[^a-zA-Z0-9_]`)
48:	uniqueSuffix = re.ReplaceAllString(uniqueSuffix, "_")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.


parent := fmt.Sprintf("organizations/%s/locations/global", orgID)

// Call Create
err = createEventThreatDetectionCustomModule(&buf, parent)
err := createEventThreatDetectionCustomModule(&buf, parent)

if err != nil {
t.Fatalf("createCustomModule() had error: %v", err)
return
}

got := buf.String()

Choose a reason for hiding this comment

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

medium

The createModulePath might be empty if the output format changes. Add a check to handle this case gracefully.

230:	got := buf.String()
231:	if got == "" {
232:		t.Errorf("createEventThreatDetectionCustomModule() returned an empty string")
233:		return
234:	}
235:	fmt.Printf("Response: %v\n", got)
236:
237:	parts := strings.Split(got, ":")
238:	if len(parts) > 0 {
239:		createModulePath = parts[len(parts)-1]
240:	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@@ -242,6 +271,14 @@ func TestCreateCustomModule(t *testing.T) {
}

got := buf.String()

Choose a reason for hiding this comment

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

medium

The createModulePath might be empty if the output format changes. Add a check to handle this case gracefully.

273:	got := buf.String()
274:	if got == "" {
275:		t.Errorf("createSecurityHealthAnalyticsCustomModule() returned an empty string")
276:		return
277:	}
278:	fmt.Printf("Response: %v\n", got)
279:
280:	parts := strings.Split(got, ":")
281:	if len(parts) > 0 {
282:		createModulePath = parts[len(parts)-1]
283:	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@vijaykanthm vijaykanthm added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 16, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 16, 2025
…ithub.com:vijaykanthm/golang-samples into fix-scc-mgt-api-sha-etd-cust-mod-test-flaky-issue

Merge remote into local
"os"
"strings"
// "sync"
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: remove commented-out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"google.golang.org/protobuf/types/known/structpb"
)

var orgID = ""
var createdCustomModuleID = ""
var (
createdModules []string
// mu sync.Mutex
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: remove commented-out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"strings"
// "sync"
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: remove commented out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

)

var orgID = ""
var createdCustomModuleID = ""
var (
createdModules []string
// mu sync.Mutex
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: remove commented out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@telpirion telpirion self-assigned this Jan 16, 2025
@vijaykanthm vijaykanthm requested a review from telpirion January 17, 2025 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: securitycenter Issues related to the Security Command Center API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

securitycenter/management_api/event_threat_detection: TestGetEtdCustomModule failed
4 participants