test(filter) : Migrate Filter test and Improve code coverage#5638
test(filter) : Migrate Filter test and Improve code coverage#5638yuvraj-kolkar17 wants to merge 2 commits intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: Yuvraj Kolkar <kolkaryuvraj2@gmail.com>
Summary of ChangesHello @yuvraj-kolkar17, 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 significantly refactors the testing infrastructure for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @yuvraj-kolkar17. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![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 successfully migrates the tests for the filter package from testify to Ginkgo and Gomega, which improves the structure and readability of the tests. The increased code coverage to 100% is also a great achievement.
I have provided two review comments on the test file. One suggests a way to improve test isolation and prevent future flakiness. The other points out a missing edge case in the tests which reveals a bug in the implementation, and I've suggested a new test case to cover it.
Additionally, please consider if the generated coverage.html file should be committed to the repository. It's common practice to add such generated files to .gitignore to keep the repository clean.
| expect := []string{"mount", "fs", "alluxio.underfs.s3.inherit.acl=false", "aws.secretKey=[ redacted ]"} | ||
| got := FilterCommand(input) | ||
| Expect(got).To(Equal(expect)) | ||
| }) |
There was a problem hiding this comment.
While the test coverage is 100%, there's an edge case not covered: when a single string contains multiple sensitive keys. The current implementation of FilterString appears to only handle one sensitive key per string, and it replaces the entire string. This can lead to incomplete redaction and non-deterministic behavior depending on map iteration order.
I suggest adding a new test case to cover this scenario. This test will likely fail, which will highlight the bug in FilterString that needs to be addressed.
})
It("should redact multiple sensitive keys in a single string", func() {
input := []string{"aws.secretKey=secret and aws.accessKeyId=key"}
// This test will fail with the current implementation, which only redacts one key
// and replaces the entire string. A correct implementation should handle this.
expect := []string{"aws.secretKey=[ redacted ] and aws.accessKeyId=[ redacted ]"}
got := FilterCommand(input)
Expect(got).To(Equal(expect))
})| var _ = Describe("FilterCommandWithSensitive", func() { | ||
| Context("when updating sensitive keys", func() { | ||
| It("should not redact keys that are not added as sensitive", func() { | ||
| filterKey := "test" | ||
| input := []string{"mount", "fs", "fs.azure.account.key=xxxxxxxxx"} | ||
| expect := []string{"mount", "fs", "fs.azure.account.key=xxxxxxxxx"} | ||
| UpdateSensitiveKey(filterKey) | ||
| got := FilterCommand(input) | ||
| Expect(got).To(Equal(expect)) | ||
| }) | ||
|
|
||
| } | ||
| It("should redact keys that are added as sensitive", func() { | ||
| filterKey := "fs.azure.account.key" | ||
| input := []string{"mount", "fs", "fs.azure.account.key=false"} | ||
| expect := []string{"mount", "fs", "fs.azure.account.key=[ redacted ]"} | ||
| UpdateSensitiveKey(filterKey) | ||
| got := FilterCommand(input) | ||
| Expect(got).To(Equal(expect)) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These tests modify the global sensitiveKeys map, which can lead to flaky tests as they are not isolated. A test modifying this global state can affect the outcome of other tests, especially if they are run in parallel or a different order in the future.
To ensure test isolation, it's a best practice to reset the state before each test. You can use a BeforeEach block to reset the sensitiveKeys map to its initial state.
var _ = Describe("FilterCommandWithSensitive", func() {
BeforeEach(func() {
// Reset sensitiveKeys to its initial state before each test
sensitiveKeys = map[string]bool{
"aws.secretKey": true,
"aws.accessKeyId": true,
"fs.oss.accessKeyId": true,
"fs.oss.accessKeySecret": true,
}
})
Context("when updating sensitive keys", func() {
It("should not redact keys that are not added as sensitive", func() {
filterKey := "test"
input := []string{"mount", "fs", "fs.azure.account.key=xxxxxxxxx"}
expect := []string{"mount", "fs", "fs.azure.account.key=xxxxxxxxx"}
UpdateSensitiveKey(filterKey)
got := FilterCommand(input)
Expect(got).To(Equal(expect))
})
It("should redact keys that are added as sensitive", func() {
filterKey := "fs.azure.account.key"
input := []string{"mount", "fs", "fs.azure.account.key=false"}
expect := []string{"mount", "fs", "fs.azure.account.key=[ redacted ]"}
UpdateSensitiveKey(filterKey)
got := FilterCommand(input)
Expect(got).To(Equal(expect))
})
})
})
yuvraj-kolkar17
force-pushed
the
test/filter-migrate-test
branch
from
18b7728 to
7ac901a
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5638 +/- ##
==========================================
+ Coverage 56.76% 59.24% +2.48%
==========================================
Files 443 444 +1
Lines 30735 30559 -176
==========================================
+ Hits 17447 18105 +658
+ Misses 11734 10954 -780
+ Partials 1554 1500 -54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the filter tests to Ginkgo and Gomega and improves the secret filtering logic. The test changes are well-structured. I have a few suggestions to improve the implementation further:
- The new filtering logic in
FilterStringis a bit complex. I've suggested a refactoring to use regular expressions, which would make the code simpler and more maintainable. - There is a potential data race on the global
sensitiveKeysmap, which I've highlighted with a suggestion to usesync.RWMutex. - The code coverage report
coverage.htmlhas been committed. This type of generated file should typically be excluded from the repository.
Overall, good work on improving the tests and functionality.
| @@ -34,14 +34,37 @@ func FilterCommand(command []string) (filteredCommand []string) { | |||
| } | |||
|
|
|||
| func FilterString(line string) string { | |||
There was a problem hiding this comment.
There is a potential data race issue. The global variable sensitiveKeys is read in this function and modified in UpdateSensitiveKey without any synchronization. If these functions are called concurrently from different goroutines, it can lead to a crash or unpredictable behavior.
To fix this, you should use a sync.RWMutex to protect all accesses to sensitiveKeys.
For example:
import "sync"
var (
sensitiveKeysMutex sync.RWMutex
sensitiveKeys = map[string]bool{...}
)
// In FilterString
sensitiveKeysMutex.RLock()
defer sensitiveKeysMutex.RUnlock()
// ...
// In UpdateSensitiveKey
sensitiveKeysMutex.Lock()
defer sensitiveKeysMutex.Unlock()
// ...| result := line | ||
| for s := range sensitiveKeys { | ||
| // if the log line contains a secret value redact it | ||
| if strings.Contains(line, s) { | ||
| line = s + "=[ redacted ]" | ||
| // if the log line contains a secret key, redact its value | ||
| if strings.Contains(result, s) { | ||
| // Look for pattern "key=" and replace everything after = until space or end | ||
| searchPattern := s + "=" | ||
| idx := strings.Index(result, searchPattern) | ||
|
|
||
| for idx != -1 { | ||
| // Find the end of the value (next space or end of string) | ||
| startValue := idx + len(searchPattern) | ||
| endValue := startValue | ||
|
|
||
| // Find where the value ends (space, newline, or end of string) | ||
| for endValue < len(result) && result[endValue] != ' ' && result[endValue] != '\n' && result[endValue] != '\t' { | ||
| endValue++ | ||
| } | ||
|
|
||
| // Replace the value with [ redacted ] | ||
| result = result[:startValue] + "[ redacted ]" + result[endValue:] | ||
|
|
||
| // Look for next occurrence | ||
| idx = strings.Index(result[startValue+len("[ redacted ]"):], searchPattern) | ||
| if idx != -1 { | ||
| idx = idx + startValue + len("[ redacted ]") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return line | ||
| return result |
There was a problem hiding this comment.
The current implementation for redacting sensitive values is functionally correct but quite complex, involving manual string searching and replacement in a loop. This makes the code hard to read and maintain.
A much simpler and more robust solution can be achieved using a single regular expression. This would also be more efficient as it processes the string in a single pass. You will need to add regexp to the file imports.
var patterns []string
for s := range sensitiveKeys {
patterns = append(patterns, regexp.QuoteMeta(s))
}
if len(patterns) == 0 {
return line
}
// Build a regex to find any of the sensitive keys followed by '=' and a value.
// The value is considered any sequence of non-whitespace characters.
// Example: (key1|key2)=<value>
re := regexp.MustCompile(`(` + strings.Join(patterns, "|") + `)=([\s]*)`)
// Replace all found occurrences with "key=[ redacted ]"
return re.ReplaceAllString(line, "$1=[ redacted ]")1 participant
Ⅰ. Describe what this PR does
Migrated your test from testify to Ginkgo + Gomega. Key changes:
Ⅱ. Does this pull request fix one issue?
Part of #5407
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews