test(creds,dentry_cache): fix portability of integration tests on non-allowlisted GCP projects#4745
test(creds,dentry_cache): fix portability of integration tests on non-allowlisted GCP projects#4745kislaykishore wants to merge 6 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness and portability of the integration and end-to-end test suites. By implementing a project-aware skipping mechanism for credential-based tests and accounting for kernel-specific error translation differences, the changes ensure that the test suite can execute reliably across diverse GCP environments without triggering unnecessary failures or panics. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
the
remind-reviewers
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates integration tests to skip execution on non-allowlisted GCP projects and introduces a helper function ValidateESTALEOrEIOError to handle cases where the FUSE kernel driver translates ESTALE to EIO. The reviewer provided valuable feedback, suggesting the use of errors.Is instead of string matching for system errors, caching the GCP project ID to prevent redundant metadata server requests, and moving the allowlist check in managed_folders_test.go earlier in TestMain to avoid bypassing deferred cleanups and unnecessary client initialization.
| func ValidateESTALEOrEIOError(t *testing.T, err error) { | ||
| t.Helper() | ||
|
|
||
| require.Error(t, err) | ||
| // FUSE kernel driver can translate ESTALE to EIO (input/output error) on some operations/kernels. | ||
| // So we accept both "stale file handle" (ESTALE) and "input/output error" (EIO). | ||
| errMsg := err.Error() | ||
| if !strings.Contains(errMsg, syscall.ESTALE.Error()) && !strings.Contains(errMsg, syscall.EIO.Error()) { | ||
| t.Fatalf("Expected error to contain %q or %q. Got: %q", syscall.ESTALE.Error(), syscall.EIO.Error(), errMsg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using strings.Contains on err.Error() to check for specific system errors like syscall.ESTALE or syscall.EIO can be fragile. In Go, the idiomatic and robust way to assert wrapped system errors (such as those wrapped in os.PathError) is using errors.Is.
func ValidateESTALEOrEIOError(t *testing.T, err error) {
t.Helper()
require.Error(t, err)
// FUSE kernel driver can translate ESTALE to EIO (input/output error) on some operations/kernels.
// So we accept both "stale file handle" (ESTALE) and "input/output error" (EIO).
if !errors.Is(err, syscall.ESTALE) && !errors.Is(err, syscall.EIO) {
t.Fatalf("Expected error to be %v or %v. Got: %v", syscall.ESTALE, syscall.EIO, err)
}
}| func IsAllowlistedProject(ctx context.Context) bool { | ||
| id, err := metadata.ProjectIDWithContext(ctx) | ||
| if err != nil { | ||
| log.Printf("Error in fetching project id: %v", err) | ||
| return false | ||
| } | ||
| if strings.Contains(id, "cloudtop") { | ||
| return true | ||
| } | ||
| return slices.Contains(AllowlistedGcpProjects, id) | ||
| } |
There was a problem hiding this comment.
Every call to metadata.ProjectIDWithContext(ctx) performs an HTTP request to the local metadata server. Since IsAllowlistedProject is called first, and then projectID is called again during credential creation, this results in redundant network roundtrips.
We can cache the fetched project ID in a package-level variable to avoid these duplicate requests. Additionally, please update the projectID(ctx) function to use fetchProjectID(ctx) instead of calling metadata.ProjectIDWithContext(ctx) directly.
var (
projectIDCache string
projectIDCacheErr error
projectIDFetched bool
)
func fetchProjectID(ctx context.Context) (string, error) {
if !projectIDFetched {
projectIDCache, projectIDCacheErr = metadata.ProjectIDWithContext(ctx)
projectIDFetched = true
}
return projectIDCache, projectIDCacheErr
}
func IsAllowlistedProject(ctx context.Context) bool {
id, err := fetchProjectID(ctx)
if err != nil {
log.Printf("Error in fetching project id: %v", err)
return false
}
if strings.Contains(id, "cloudtop") {
return true
}
return slices.Contains(AllowlistedGcpProjects, id)
}| if !creds_tests.IsAllowlistedProject(testEnv.ctx) { | ||
| log.Printf("The active GCP project is not one of the allowlisted projects. Skipping managed folders tests.") | ||
| os.Exit(0) | ||
| } |
There was a problem hiding this comment.
This check is currently placed after the storage and control clients are created. If the project is not allowlisted, os.Exit(0) is called, which bypasses the deferred cleanup functions for those clients.
Additionally, initializing the clients is unnecessary if the tests are going to be skipped.
Consider moving this check to the beginning of TestMain (right after testEnv.cfg is initialized, around line 92), before the clients are created.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4745 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…underlying system errors
1 participant
Description
The FUSE kernel driver can translate ESTALE to EIO on certain versions. Make tests more robust to handle it. Specifically:
ValidateESTALEOrEIOErrorhelper to accept bothsyscall.ESTALE("stale file handle") andsyscall.EIO("input/output error"). This accounts for differences in how the Linux FUSE driver translates FUSEESTALEerrors to the userspace depending on the kernel environment/version. Updated thedentry_cacheandnotifier_teststo use the method.Link to the issue in case of a bug fix.
b/518338525
Testing details
make e2e-teston a VM hosted in a non-allowlisted project successfully runs all portable tests and gracefully skips credentials-based tests without crashes. All run tests passed.dentry_cache,mount_timeout,requester_pays_bucket, andmanaged_folderstests execute successfully.Any backward incompatible change? If so, please explain.
No backward incompatible changes.