feat: getter of kernel and NFS mount config #88
Conversation
WalkthroughThe changes introduced in this pull request enhance the functionality for managing NFS configurations and kernel configurations. The Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
ns/sys.go (1)
101-103: Improve function documentationThe documentation should be enhanced to include:
- Return value description
- Possible error scenarios
- Example usage
Consider updating the documentation like this:
-// GetProcKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from -// procfs mounted at procPrefix from the view of processName in namespace. If the procPrefix is empty, it points to /proc by default. +// GetProcKernelConfigMap reads the kernel configuration into a key-value map from the procfs. +// +// Parameters: +// - procPrefix: Custom procfs mount point. If empty, defaults to /proc +// - processName: Name of the process to execute from +// - namespaces: List of namespaces to execute in +// +// Returns: +// - map[string]string: Kernel configuration key-value pairs +// - error: If reading or parsing fails, or if zcat is not available +// +// Example: +// config, err := GetProcKernelConfigMap("", "longhorn-manager", []types.Namespace{types.NamespaceHost})nfs/nfs.go (2)
13-19: Add documentation for constantsConsider adding comments to explain the purpose of each constant, especially the default version values and their significance.
const ( + // nfsmountGlobalSection is the section name for global options in nfsmount.conf nfsmountGlobalSection = "NFSMount_Global_Options" + // nfsmountDefaultVersion is the key name for default version in nfsmount.conf nfsmountDefaultVersion = "Defaultvers" + // defaultNFSVersionMajor is the fallback major version when no config is found defaultNFSVersionMajor = 4 + // defaultNFSVersionMinor is the fallback minor version when no config is found defaultNFSVersionMinor = 0 )
21-23: Enhance function documentation with examplesConsider adding examples to the documentation to show expected input/output scenarios.
// GetSystemDefaultNFSVersion reads the system default NFS version. This config can be overridden by nfsmount.conf under // configPrefix. If configPrefix is empty, it will be /etc by default. +// +// Example: +// major, minor, err := GetSystemDefaultNFSVersion("") // reads from /etc/nfsmount.conf +// major, minor, err := GetSystemDefaultNFSVersion("/custom/path") // reads from /custom/path/nfsmount.confnfs/nfs_test.go (3)
21-25: Simplify string construction in helper functionThe
fmt.Sprintfusage can be simplified since we're just concatenating strings.- data := fmt.Sprintf("[ NFSMount_Global_Options ]\nDefaultvers=%s\n", nfsVer) + data := "[ NFSMount_Global_Options ]\nDefaultvers=" + nfsVer + "\n"
33-89: Well-structured test cases with good coverageThe test cases cover important scenarios including:
- Missing configuration
- Configuration without version
- Valid versions (3, 4, 4.0, 4.2)
- Invalid versions
Consider adding test cases for:
- Malformed configuration file
- Version numbers outside valid range (e.g., "5.0", "2.0")
🧰 Tools
🪛 golangci-lint (1.62.2)
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
91-110: Consider handling cleanup errorsThe cleanup using
os.Removeshould check for errors, especially in test code where we want to be aware of any issues.- defer os.Remove(filepath.Join(configDir, "nfsmount.conf")) + defer func() { + if err := os.Remove(filepath.Join(configDir, "nfsmount.conf")); err != nil { + c.Logf("Failed to cleanup test file: %v", err) + } + }()sys/sys_test.go (1)
155-169: Consider adding more comprehensive test cases.While the current test case covers basic kernel config parsing, consider adding:
- Edge cases:
- Empty file
- Malformed config entries
- Lines without values
- Additional NFS-related configs that would be relevant for the NFS version checking feature
Here's a suggested expansion of test cases:
testCases := map[string]testCase{ "GetBootKernelConfigMap(...): read kernel config": { mockFileContent: `CONFIG_DM_CRYPT=y # comment should be ignored CONFIG_NFS_V4=m CONFIG_NFS_V4_1=m CONFIG_NFS_V4_2=y`, expected: map[string]string{ "CONFIG_DM_CRYPT": "y", "CONFIG_NFS_V4": "m", "CONFIG_NFS_V4_1": "m", "CONFIG_NFS_V4_2": "y", }, }, + "GetBootKernelConfigMap(...): empty file": { + mockFileContent: "", + expected: map[string]string{}, + }, + "GetBootKernelConfigMap(...): malformed entries": { + mockFileContent: `CONFIG_NFS_V4 +CONFIG_INVALID==value +CONFIG_VALID=m`, + expected: map[string]string{ + "CONFIG_VALID": "m", + }, + }, + "GetBootKernelConfigMap(...): additional NFS configs": { + mockFileContent: `CONFIG_NFS_FS=y +CONFIG_NFS_V3=y +CONFIG_NFS_V4=m +CONFIG_NFS_V4_1=m +CONFIG_NFS_V4_2=y +CONFIG_NFS_FSCACHE=y`, + expected: map[string]string{ + "CONFIG_NFS_FS": "y", + "CONFIG_NFS_V3": "y", + "CONFIG_NFS_V4": "m", + "CONFIG_NFS_V4_1": "m", + "CONFIG_NFS_V4_2": "y", + "CONFIG_NFS_FSCACHE": "y", + }, + }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
nfs/nfs.go(1 hunks)nfs/nfs_test.go(1 hunks)ns/sys.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
nfs/nfs_test.go
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
sys/sys_test.go
177-177: Error return value of os.WriteFile is not checked
(errcheck)
🔇 Additional comments (6)
nfs/nfs.go (2)
1-11: LGTM! Package structure and imports are well organized.
1-83: Verify integration with preflight checker and node controller
The implementation provides the necessary functionality for checking NFS versions, but we should verify its integration with the preflight checker and node controller as mentioned in the PR objectives.
nfs/nfs_test.go (1)
1-19: LGTM! Clean test setup following standard patterns
The test setup follows best practices using check.v1 framework and includes necessary imports.
sys/sys_test.go (1)
150-154: LGTM! Test structure follows established patterns.
The test case structure is well-defined and consistent with other tests in the file.
sys/sys.go (2)
4-4: LGTM: Import changes are appropriate
The new imports are properly organized and justified by their usage in the new functions.
Also applies to: 15-15
133-162: Verify kernel config format and NFS-related keys
Since this implementation is part of the NFS version checking feature, we should verify:
- The actual format of kernel config files matches our parsing logic
- All necessary NFS-related configuration keys are captured
✅ Verification successful
Let me gather more information about the kernel config format by searching through the codebase for test files and examples.
Let me check the actual implementation and usage of these functions in the codebase to understand the context better.
Implementation correctly handles kernel config format and NFS-related keys
The implementation is verified to be correct based on the following evidence:
- The test file
sys/sys_test.gocontains proper test cases that validate the parsing logic with sample kernel config data including commented lines. - The actual NFS-related config entries found in the codebase (
CONFIG_NFS_V4=m,CONFIG_NFS_V4_1=m,CONFIG_NFS_V4_2=y) follow the exact format that our parsing logic handles:CONFIG_*=value - The implementation correctly:
- Reads from the standard kernel config location (
/boot/config-{version}) - Ignores comments and non-CONFIG lines
- Properly splits on '=' and trims whitespace
- Handles both module ('m') and built-in ('y') config values
- Reads from the standard kernel config location (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify kernel config format and check for NFS-related keys
# Check format of actual kernel config file
echo "=== Kernel Config Format ==="
if [ -f /boot/config-$(uname -r) ]; then
head -n 10 /boot/config-$(uname -r)
fi
# Look for NFS-related config entries
echo -e "\n=== NFS-Related Config Entries ==="
rg "CONFIG_NFS" --no-filename
# Check for any non-standard CONFIG_ entries that might break our parsing
echo -e "\n=== Unusual CONFIG_ Format ==="
rg "^CONFIG_.*[^=]$|^CONFIG_.*=.*=.*$" --no-filename
Length of output: 549
Script:
#!/bin/bash
# Search for test files related to kernel config parsing
echo "=== Test Files ==="
fd -e go -e txt test.*kernel.*config
# Search for test data or example kernel configs
echo -e "\n=== Test Data Content ==="
rg -A 5 "CONFIG_.*=" testdata/ || true
# Look for any existing test cases for these functions
echo -e "\n=== Test Cases ==="
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
GetBootKernelConfigMap($$$)
$$$
}'
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
ParseKernelModuleConfigMap($$$)
$$$
}'
Length of output: 491
Script:
#!/bin/bash
# Find where these functions are used
echo "=== Function Usage ==="
rg "GetBootKernelConfigMap|ParseKernelModuleConfigMap" -B 2 -A 2
# Look for any kernel config related constants or test data
echo -e "\n=== Kernel Config Related Code ==="
rg "config-.*\." --type go
# Check if there are any other kernel config parsing functions
echo -e "\n=== Related Functions ==="
ast-grep --pattern 'func $_($_) (map[string]string, error) {
$$$
}'
Length of output: 5999
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
9830-check_nfs_version_in_nfsmount
branch
from
9948ac2
to
2496ae7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
ns/sys.go (3)
101-103: Enhance function documentation with parameter descriptions and return valuesConsider adding more detailed documentation:
// GetProcKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from // procfs mounted at procPrefix from the view of processName in namespace. If the procPrefix is empty, it points to /proc by default. +// +// Parameters: +// - procPrefix: The path prefix where procfs is mounted (defaults to "/proc" if empty) +// - processName: The name of the process to execute from +// - namespaces: List of namespaces to execute within +// +// Returns: +// - configMap: Map of kernel configuration key-value pairs +// - err: Error if any step fails
108-110: Improve path normalization for procPrefixThe current path handling could lead to double slashes. Consider using
path.Clean:if procPrefix == "" { - procPrefix = "/proc/" + procPrefix = "/proc" } +procPrefix = path.Clean(procPrefix)
101-122: Consider caching kernel config resultsSince kernel configs rarely change during runtime, consider implementing a caching mechanism to avoid repeated reads and parsing of the config file. This would be especially beneficial when the function is called frequently during NFS version checks.
Example approach:
- Add a package-level cache variable
- Implement cache invalidation on specific events (if needed)
- Add a force refresh parameter for cases where fresh data is required
nfs/nfs_test.go (4)
21-25: Simplify string formatting in helper function.The string formatting can be simplified since it's a basic concatenation.
- data := fmt.Sprintf("[ NFSMount_Global_Options ]\nDefaultvers=%s\n", nfsVer) + data := "[ NFSMount_Global_Options ]\nDefaultvers=" + nfsVer + "\n"
27-89: Consider adding more edge cases to test suite.The test cases are comprehensive but could benefit from additional scenarios:
- Malformed configuration file (invalid syntax)
- Permission issues (read-only directory or file)
- Version numbers with leading/trailing spaces
Would you like me to provide example implementations for these additional test cases?
🧰 Tools
🪛 golangci-lint (1.62.2)
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
100-107: Enhance error messages with more context.Consider adding more detailed error messages that include the actual values received when assertions fail.
- c.Assert(major, Equals, testCase.expectedMajor, Commentf(test.ErrResultFmt, testName)) - c.Assert(minor, Equals, testCase.expectedMinor, Commentf(test.ErrResultFmt, testName)) + c.Assert(major, Equals, testCase.expectedMajor, Commentf("%s: expected major version %d, got %d", testName, testCase.expectedMajor, major)) + c.Assert(minor, Equals, testCase.expectedMinor, Commentf("%s: expected minor version %d, got %d", testName, testCase.expectedMinor, minor))
1-110: Test coverage aligns well with PR objectives.The test suite provides comprehensive coverage for reading NFS versions from the configuration file, which supports both the preflight checker and node controller requirements mentioned in the PR objectives. The test cases cover various scenarios that these components might encounter.
Consider documenting the relationship between this common logic and its usage in the preflight checker and node controller, perhaps through code comments or package documentation.
🧰 Tools
🪛 golangci-lint (1.62.2)
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
sys/sys_test.go (1)
171-184: Add cleanup for temporary files.The test creates temporary files but doesn't clean them up. While the OS might handle this, it's better to explicitly clean up test artifacts.
Add cleanup after the test:
bootDir := c.MkDir() +defer os.RemoveAll(bootDir) kernelVersion := "1.2.3.4"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
nfs/nfs.go(1 hunks)nfs/nfs_test.go(1 hunks)ns/sys.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nfs/nfs.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
nfs/nfs_test.go
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
🔇 Additional comments (9)
ns/sys.go (2)
112-116: LGTM! Namespace executor creation is well implemented
The code correctly creates and handles the namespace executor with proper error checking.
117-121: Fix kernel config reading implementation
This implementation has several issues:
- Uses hardcoded "/proc/config.gz" instead of using the provided procPrefix
- No fallback mechanism if zcat is unavailable
- No verification if the config file exists
This was previously identified in an earlier review. Please refer to the existing comment for the detailed fix.
Additionally, consider implementing a fallback to GetBootKernelConfigMap as suggested in the previous codebase verification comment.
nfs/nfs_test.go (1)
1-19: LGTM! Clean test setup following Go best practices.
The test setup properly initializes the testing framework with appropriate imports and test suite structure.
sys/sys_test.go (2)
150-154: LGTM!
The test case structure is well-defined and consistent with other test functions in the file.
177-178: Use consistent error handling style.
The error handling style should match the pattern used elsewhere in the file.
Apply this diff to maintain consistency:
-err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
-c.Assert(err, IsNil)
+err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
+c.Assert(err, IsNil, Commentf(test.ErrErrorFmt, testName, err))sys/sys.go (4)
4-4: LGTM: Import changes are appropriate
The new imports are necessary for the added functionality and are properly organized.
Also applies to: 15-15
133-150:
Add security measures and improve documentation
While the basic validation is good, there are several security and documentation improvements needed:
- Add path traversal protection:
func GetBootKernelConfigMap(bootPrefix, kernelVersion string) (map[string]string, error) {
if kernelVersion == "" {
return nil, fmt.Errorf("kernelVersion cannot be empty")
}
+ if strings.Contains(kernelVersion, "..") || strings.Contains(kernelVersion, "/") {
+ return nil, fmt.Errorf("invalid kernelVersion: must not contain path traversal sequences")
+ }
if bootPrefix == "" {
bootPrefix = "/boot/"
}
+ bootPrefix = filepath.Clean(bootPrefix)- Improve documentation:
-// GetBootKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from
-// ${bootPrefix}/config-${kernelVersion}, and comments are ignored. If the bootPrefix is empty, it points to /boot by
-// default.
+// GetBootKernelConfigMap reads the kernel configuration into a key-value map from the specified boot directory.
+// Parameters:
+// - bootPrefix: The directory containing kernel configs (defaults to "/boot/" if empty)
+// - kernelVersion: The kernel version string (must not be empty or contain path separators)
+// Returns:
+// - map[string]string: Key-value pairs of kernel configuration
+// - error: If the file cannot be read or the version string is invalid
+// The function constructs the path ${bootPrefix}/config-${kernelVersion} to read the configuration.
+// Example: GetBootKernelConfigMap("/boot", "4.15.0-72-generic")152-170:
Improve error handling and performance
The function needs more robust error handling and performance optimizations:
func ParseKernelModuleConfigMap(kernelConfigContent string) (map[string]string, error) {
- configMap := map[string]string{}
+ // Pre-allocate map with estimated size (assuming ~1000 config entries)
+ configMap := make(map[string]string, 1000)
scanner := bufio.NewScanner(strings.NewReader(kernelConfigContent))
for scanner.Scan() {
config := scanner.Text()
if !strings.HasPrefix(config, "CONFIG_") {
continue
}
configSplits := strings.Split(config, "=")
+ if len(configSplits) != 2 {
+ return nil, fmt.Errorf("malformed config line: %s", config)
+ }
+ key := strings.TrimSpace(configSplits[0])
+ value := strings.TrimSpace(configSplits[1])
+ if key == "" || value == "" {
+ return nil, fmt.Errorf("invalid config line: empty key or value in %s", config)
+ }
- configMap[strings.TrimSpace(configSplits[0])] = strings.TrimSpace(configSplits[1])
+ configMap[key] = value
}
if err := scanner.Err(); err != nil {
- return nil, err
+ return nil, fmt.Errorf("error scanning config: %w", err)
}
return configMap, nil
}Additionally, consider adding validation for known config value formats (e.g., "y", "n", "m" for module configs).
133-170: Verify NFS version detection capabilities
Since this code will be used for NFS version checking, let's verify it can properly detect NFS-related kernel configs:
✅ Verification successful
Let me check the test file to understand how NFS version detection is being tested.
Implementation correctly supports NFS version detection
The implementation properly handles NFS kernel configuration detection as evidenced by the test cases. The code can successfully parse different NFS version configurations (V4, V4.1, V4.2) with their respective module states ('m' for module, 'y' for built-in).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the implementation can detect NFS-related kernel configs
# Search for NFS-related kernel config patterns in the codebase
echo "Searching for NFS kernel config usage patterns..."
rg -l 'CONFIG_NFS_V\d'
# Search for NFS version checks in test files
echo "Searching for NFS version test cases..."
rg -l 'TestGetBootKernelConfigMap.*NFS' 'test'
# Check if there are any NFS-related constants defined
echo "Searching for NFS-related constants..."
ast-grep --pattern 'const.*NFS.*Version'
Length of output: 415
Script:
#!/bin/bash
# Check the test implementation for NFS version detection
rg -A 10 'CONFIG_NFS_V\d' sys/sys_test.go
Length of output: 392
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
sys/sys.go (1)
152-173: Improve error handling and code styleThe function needs some improvements in error handling and code structure:
// ParseKernelModuleConfigMap parses the kernel config into key-value map. All commented items will be ignored. +// +// The function expects kernel config entries in the format: CONFIG_KEY=VALUE +// Lines not starting with CONFIG_ are ignored. func ParseKernelModuleConfigMap(kernelConfigContent string) (map[string]string, error) { + if strings.TrimSpace(kernelConfigContent) == "" { + return nil, fmt.Errorf("empty kernel config content") + } configMap := map[string]string{} scanner := bufio.NewScanner(strings.NewReader(kernelConfigContent)) for scanner.Scan() { config := scanner.Text() if !strings.HasPrefix(config, "CONFIG_") { continue } - if key, val, parsable := strings.Cut(config, "="); !parsable { - return nil, fmt.Errorf("failed to parse kernel config %s", config) - } else { - configMap[strings.TrimSpace(key)] = strings.TrimSpace(val) + key, val, parsable := strings.Cut(config, "=") + if !parsable { + return nil, fmt.Errorf("invalid format in kernel config line: %q", config) } + key, val = strings.TrimSpace(key), strings.TrimSpace(val) + if key == "" || val == "" { + return nil, fmt.Errorf("empty key or value in kernel config line: %q", config) + } + configMap[key] = val } if err := scanner.Err(); err != nil { - return nil, err + return nil, fmt.Errorf("error scanning kernel config: %w", err) } return configMap, nil }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)sys/sys_test.go (1)
156-181: Consider adding more error scenarios.While the current test cases cover basic scenarios, consider adding:
- Missing config file scenario
- Permission denied scenario (read-only file)
Here's a suggested addition to the test cases:
testCases := map[string]testCase{ // ... existing test cases ... + "GetBootKernelConfigMap(...): missing config file": { + mockFileContent: "", + expectedConfigMap: nil, + expectedError: true, + // Set kernelVersion to a non-existent version in the test + }, + "GetBootKernelConfigMap(...): permission denied": { + mockFileContent: "CONFIG_TEST=y", + expectedConfigMap: nil, + expectedError: true, + // Test will need to chmod the file to read-only + }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
nfs/nfs_test.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nfs/nfs_test.go
🧰 Additional context used
🪛 GitHub Check: CodeFactor
sys/sys.go
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)
🔇 Additional comments (5)
sys/sys.go (3)
4-4: LGTM: Required imports added
The new imports are necessary for the added functionality:
bufiofor efficient line scanninggo-common-libs/iofor file operations
Also applies to: 15-15
133-173: Verify kernel config parsing for NFS entries
Since this code will be used for NFS version checking, let's verify the parsing of NFS-related kernel configs.
✅ Verification successful
Kernel config parsing implementation correctly handles NFS version configs
The implementation correctly parses kernel config entries including NFS-related configurations. The test cases demonstrate proper handling of:
- NFS version config entries (CONFIG_NFS_V4, CONFIG_NFS_V4_1, CONFIG_NFS_V4_2)
- Different config values ('m' for modules and 'y' for built-in)
- Comment filtering
- Key-value parsing with proper trimming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the format of kernel config files and presence of NFS-related entries
# Check format of kernel config files in /boot
echo "=== Checking kernel config format ==="
fd 'config-' /boot --type f --exec head -n 5 {} \;
# Search for NFS-related kernel config entries
echo -e "\n=== Checking NFS-related kernel configs ==="
fd 'config-' /boot --type f --exec grep -H 'CONFIG_NFS' {} \;
Length of output: 490
Script:
#!/bin/bash
# Let's verify the kernel config parsing logic by searching for test files and examples in the codebase
# Search for test files related to kernel config parsing
echo "=== Searching for kernel config test files ==="
rg -l "ParseKernelModuleConfigMap|GetBootKernelConfigMap" --type go
# Search for test data or example kernel configs
echo -e "\n=== Searching for test data or example configs ==="
fd -e config -e conf --exec head -n 10 {} \;
# Look for any NFS-related test cases
echo -e "\n=== Searching for NFS-related test cases ==="
rg "CONFIG_NFS" -A 5 -B 5
Length of output: 1280
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)
133-150: 🛠️ Refactor suggestion
Add security checks and improve documentation
While the function handles basic validation, it needs additional security measures and documentation improvements:
- Add path traversal prevention for both parameters
- Enhance error messages with context
- Document the return value format
-// GetBootKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from
-// ${bootPrefix}/config-${kernelVersion}, and comments are ignored. If the bootPrefix is empty, it points to /boot by
-// default.
+// GetBootKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from
+// ${bootPrefix}/config-${kernelVersion}, and comments are ignored. If the bootPrefix is empty, it points to /boot by
+// default.
+//
+// Returns:
+// - map[string]string: Key-value pairs where keys are CONFIG_* kernel parameters
+// - error: If file cannot be read or contains invalid format
func GetBootKernelConfigMap(bootPrefix, kernelVersion string) (map[string]string, error) {
if kernelVersion == "" {
- return nil, fmt.Errorf("kernelVersion cannot be empty")
+ return nil, fmt.Errorf("invalid input: kernelVersion cannot be empty")
+ }
+ if strings.Contains(kernelVersion, "..") || strings.Contains(kernelVersion, "/") {
+ return nil, fmt.Errorf("invalid input: kernelVersion contains invalid characters")
}
if bootPrefix == "" {
bootPrefix = "/boot/"
+ } else if strings.Contains(bootPrefix, "..") {
+ return nil, fmt.Errorf("invalid input: bootPrefix contains path traversal sequences")
}
configPath := filepath.Join(bootPrefix, "config-"+kernelVersion)
configContent, err := io.ReadFileContent(configPath)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to read kernel config from %s: %w", configPath, err)
}
return ParseKernelModuleConfigMap(configContent)
}Likely invalid or redundant comment.
sys/sys_test.go (2)
150-155: LGTM! Well-structured test setup.
The test case structure is clear and includes all necessary fields for comprehensive testing.
189-190: Enhance error handling for file operations.
The error handling for os.WriteFile should include cleanup in case of failure.
Apply this diff to improve error handling:
- err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
- c.Assert(err, IsNil)
+ configPath := filepath.Join(bootDir, "config-"+kernelVersion)
+ err := os.WriteFile(configPath, []byte(testCase.mockFileContent), 0644)
+ c.Assert(err, IsNil, Commentf(test.ErrErrorFmt, testName, err))
+ defer os.Remove(configPath)
COLDTURNIP
force-pushed
the
9830-check_nfs_version_in_nfsmount
branch
from
a0b366d
to
fd5fb13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
nfs/nfs_test.go (2)
21-25: Simplify string concatenation in helper function.The
fmt.Sprintfusage is unnecessary for simple string concatenation.- data := fmt.Sprintf("[ NFSMount_Global_Options ]\nDefaultvers=%s\n", nfsVer) + data := "[ NFSMount_Global_Options ]\nDefaultvers=" + nfsVer + "\n"
33-89: Consider adding edge cases to test suite.The test cases are comprehensive for normal operations, but consider adding these scenarios:
- Malformed configuration file (invalid syntax)
- Permission denied when reading the config file
Example additional test case:
"GetSystemDefaultNFSVersion(...): malformed config file": { setup: func(prefix string) { data := "invalid [ syntax" err := os.WriteFile(filepath.Join(prefix, "nfsmount.conf"), []byte(data), 0644) c.Assert(err, IsNil) }, expectedMajor: 0, expectedMinor: 0, expectedError: true, },🧰 Tools
🪛 golangci-lint (1.62.2)
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
sys/sys.go (3)
133-135: Enhance documentation with config format exampleThe comment would be more helpful with an example of the expected kernel config file format.
Add example format to the documentation:
// GetBootKernelConfigMap reads the kernel config into a key-value map. It tries to read kernel config from // ${bootPrefix}/config-${kernelVersion}, and comments are ignored. If the bootPrefix is empty, it points to /boot by -// default. +// default. +// +// Example config format: +// CONFIG_NFS_V4=y +// CONFIG_NFS_V4_1=y +// CONFIG_NFS_V4_2=y
162-166: Simplify control flow by removing unnecessary elseThe code can be simplified by removing the else block as suggested by static analysis.
- if key, val, parsable := strings.Cut(config, "="); !parsable { - return nil, fmt.Errorf("failed to parse kernel config %s", config) - } else { - configMap[strings.TrimSpace(key)] = strings.TrimSpace(val) - } + key, val, parsable := strings.Cut(config, "=") + if !parsable { + return nil, fmt.Errorf("failed to parse kernel config %s", config) + } + configMap[strings.TrimSpace(key)] = strings.TrimSpace(val)🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)
152-173: Add validation for kernel config valuesConsider adding validation for common kernel config values (y/n/m) to catch potential parsing issues early.
func ParseKernelModuleConfigMap(kernelConfigContent string) (map[string]string, error) { configMap := map[string]string{} + validValues := map[string]bool{"y": true, "n": true, "m": true} scanner := bufio.NewScanner(strings.NewReader(kernelConfigContent)) for scanner.Scan() { config := scanner.Text() if !strings.HasPrefix(config, "CONFIG_") { continue } key, val, parsable := strings.Cut(config, "=") if !parsable { return nil, fmt.Errorf("failed to parse kernel config %s", config) } + val = strings.TrimSpace(val) + if strings.HasPrefix(val, "\"") { + // Skip validation for string values + configMap[strings.TrimSpace(key)] = val + continue + } + if !validValues[val] { + return nil, fmt.Errorf("invalid kernel config value in %s", config) + } configMap[strings.TrimSpace(key)] = val } if err := scanner.Err(); err != nil { return nil, err } return configMap, nil }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)sys/sys_test.go (2)
156-181: Enhance test coverage with additional edge cases.While the current test cases cover basic scenarios, consider adding the following cases:
- Non-existent config file
- Permission denied scenario
- Malformed CONFIG entries (e.g.,
CONFIG_NFS_V4=invalid_value)Here's a suggested addition to the test cases:
testCases := map[string]testCase{ + "GetBootKernelConfigMap(...): non-existent config": { + mockFileContent: "", + expectedConfigMap: nil, + expectedError: true, + }, + "GetBootKernelConfigMap(...): malformed config entry": { + mockFileContent: "CONFIG_NFS_V4=invalid\nCONFIG_NFS_V4_1=", + expectedConfigMap: nil, + expectedError: true, + }, // ... existing test cases ... }
183-199: Add cleanup for test resources.The test should clean up the temporary files after execution.
Add a defer statement to clean up the test directory:
bootDir := c.MkDir() kernelVersion := "1.2.3.4" +defer func() { + if err := os.RemoveAll(bootDir); err != nil { + c.Logf("Failed to cleanup test directory: %v", err) + } +}()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
nfs/nfs_test.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
nfs/nfs_test.go
45-45: S1039: unnecessary use of fmt.Sprintf
(gosimple)
🪛 GitHub Check: CodeFactor
sys/sys.go
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)
🔇 Additional comments (6)
nfs/nfs_test.go (1)
97-98: Ensure cleanup runs even when tests fail.
The cleanup might not execute if testCase.setup fails. Move the cleanup defer statement before the setup call.
- defer os.Remove(filepath.Join(configDir, "nfsmount.conf"))
- testCase.setup(configDir)
+ defer os.Remove(filepath.Join(configDir, "nfsmount.conf"))
+ testCase.setup(configDir)sys/sys.go (3)
4-4: LGTM: Import additions are appropriate
The new imports are correctly added and necessary for the implemented functionality.
Also applies to: 15-15
133-150:
Add path traversal protection
The function needs additional security checks to prevent path traversal attacks through the kernelVersion parameter.
133-173: Verify integration with NFS version checking
Let's verify that the kernel config parsing correctly handles NFS-related configurations.
✅ Verification successful
Let me check the test implementation to better understand how NFS configs are handled.
Let me try a different approach to check the test implementation.
Implementation correctly handles NFS configuration parsing
The implementation properly handles NFS-related kernel configurations as evidenced by the test cases. The ParseKernelModuleConfigMap function correctly:
- Parses NFS version configurations (V4, V4.1, V4.2)
- Handles different config values ('m' for modules and 'y' for built-in)
- Ignores comments in the config file
- Maintains the exact config key format (CONFIG_NFS_*)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for NFS-related kernel config parsing
# Test: Search for NFS config patterns in kernel config files
# Look for kernel config files
echo "Searching for kernel config files..."
fd "config-" /boot --type f --exec head -n 20 {} \; | rg "CONFIG_NFS"
# Search for NFS-related patterns in the codebase
echo "Checking NFS config usage in codebase..."
rg "CONFIG_NFS" --type go
Length of output: 597
Script:
#!/bin/bash
# Get the full context of the test file
ast-grep --pattern 'func Test$_($_) {
$$$
CONFIG_NFS$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Get the content of the test file focusing on NFS config tests
rg "CONFIG_NFS" sys/sys_test.go -B 5 -A 5
Length of output: 526
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 164-164: sys/sys.go#L164
If block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (indent-error-flow)
sys/sys_test.go (2)
150-155: LGTM! Well-structured test setup.
The test case structure is clear and follows the established pattern in the codebase.
189-191: Improve error handling for file operations.
The error from os.WriteFile should be checked with a descriptive error message.
Apply this diff to improve error handling:
-err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
-c.Assert(err, IsNil)
+err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
+c.Assert(err, IsNil, Commentf("Failed to write test config file for %v: %v", testName, err))
COLDTURNIP
force-pushed
the
9830-check_nfs_version_in_nfsmount
branch
from
fd5fb13
to
3181b79
Compare
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
9830-check_nfs_version_in_nfsmount
branch
from
3181b79
to
0c62595
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
=========================================
+ Coverage 0 77.07% +77.07%
=========================================
Files 0 37 +37
Lines 0 2068 +2068
=========================================
+ Hits 0 1594 +1594
- Misses 0 344 +344
- Partials 0 130 +130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
sys/sys.go (1)
152-173: Add value validation and improve documentationThe function efficiently parses the config but could benefit from additional validation and documentation:
Consider these improvements:
-// ParseKernelModuleConfigMap parses the kernel config into key-value map. All commented items will be ignored. +// ParseKernelModuleConfigMap parses the kernel config into key-value map. +// Format: CONFIG_KEY=VALUE +// Lines not starting with CONFIG_ are ignored. +// Returns: +// - map[string]string: Parsed configuration key-value pairs +// - error: If parsing fails or invalid format is encountered func ParseKernelModuleConfigMap(kernelConfigContent string) (map[string]string, error) { configMap := map[string]string{} scanner := bufio.NewScanner(strings.NewReader(kernelConfigContent)) for scanner.Scan() { config := scanner.Text() if !strings.HasPrefix(config, "CONFIG_") { continue } key, val, parsable := strings.Cut(config, "=") if !parsable { return nil, fmt.Errorf("failed to parse kernel config %s", config) } + key = strings.TrimSpace(key) + val = strings.TrimSpace(val) + if val == "" { + return nil, fmt.Errorf("empty value in kernel config %s", config) + } - configMap[strings.TrimSpace(key)] = strings.TrimSpace(val) + configMap[key] = val }sys/sys_test.go (4)
156-181: Add test cases for additional edge cases.While the current test cases cover basic scenarios, consider adding tests for:
- Malformed CONFIG entries (e.g., "CONFIG_NFS_V4=invalid_value")
- Duplicate CONFIG entries
- Case sensitivity handling
testCases := map[string]testCase{ // ... existing test cases ... + "GetBootKernelConfigMap(...): malformed value": { + mockFileContent: "CONFIG_NFS_V4=invalid_value\n", + expectedConfigMap: nil, + expectedError: true, + }, + "GetBootKernelConfigMap(...): duplicate entries": { + mockFileContent: "CONFIG_NFS_V4=m\nCONFIG_NFS_V4=y\n", + expectedConfigMap: nil, + expectedError: true, + }, + "GetBootKernelConfigMap(...): case sensitivity": { + mockFileContent: "config_nfs_v4=m\n", + expectedConfigMap: nil, + expectedError: true, + }, }
183-185: Add cleanup for test directory.Consider adding a deferred cleanup to ensure the test directory is removed after the test:
bootDir := c.MkDir() +defer os.RemoveAll(bootDir) kernelVersion := "1.2.3.4"
189-191: Use constant for file permissions.Consider defining a constant for the file permissions instead of using a magic number:
+const defaultTestFileMode = 0644 + -err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644) +err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), defaultTestFileMode) c.Assert(err, IsNil)
192-198: Improve error message in assertions.The error assertions could be more descriptive to help with debugging:
exact, err := GetBootKernelConfigMap(bootDir, kernelVersion) -c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf(test.ErrResultFmt, testName)) +c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf("Test %q: expected config map %v, got %v", testName, testCase.expectedConfigMap, exact)) if testCase.expectedError { - c.Assert(err, NotNil, Commentf(test.ErrErrorFmt, testName, err)) + c.Assert(err, NotNil, Commentf("Test %q: expected an error but got nil", testName)) } else { - c.Assert(err, IsNil) + c.Assert(err, IsNil, Commentf("Test %q: unexpected error: %v", testName, err)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
nfs/nfs_test.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nfs/nfs_test.go
🔇 Additional comments (3)
sys/sys.go (2)
4-4: LGTM: Imports are appropriate
The new imports are necessary for the added functionality - bufio for efficient line scanning and go-common-libs/io for file operations.
Also applies to: 15-15
133-173: Verify integration with NFS version checking
The implementation provides the foundation for reading kernel configurations, but let's verify it meets the NFS version checking requirements:
✅ Verification successful
Let me gather more information about the NFS test file to understand how this kernel config parsing functionality is being utilized.
Based on the gathered information, I can now generate the final response:
Implementation aligns with NFS version checking requirements
The kernel config parsing implementation in sys.go is well-suited for NFS version checking as evidenced by:
- Test cases in
sys/sys_test.goalready include NFS-related kernel configs (CONFIG_NFS_V4,CONFIG_NFS_V4_1,CONFIG_NFS_V4_2) - The
ParseKernelModuleConfigMapfunction correctly handles the format of NFS kernel configurations - The NFS package's implementation in
nfs/nfs.gofocuses on reading system-wide NFS configuration, which complements the kernel config parsing functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the kernel config parsing can handle NFS-related configurations
# Test 1: Check if there are any NFS-related kernel configs in the codebase
echo "Checking for NFS-related kernel configs..."
rg "CONFIG_NFS" -A 3
# Test 2: Check if there are any existing NFS version checks
echo "Checking for NFS version checks..."
rg "nfsvers|nfs_vers" -A 3
# Test 3: Look for test files that might need updates
echo "Checking for related test files..."
fd -g "*nfs*_test.go"
Length of output: 761
Script:
#!/bin/bash
# Check the content of NFS test file and related implementation
echo "Checking NFS test implementation..."
cat nfs/nfs_test.go
# Look for the main NFS implementation file
echo -e "\nChecking NFS implementation..."
fd -g "nfs.go" --type f | grep -v "_test" | xargs cat
Length of output: 6417
sys/sys_test.go (1)
150-155: LGTM! Well-structured test setup.
The test case structure is clear and follows the established pattern in the test suite.
|
Hi @derekbit @c3y1huang, need your help to review this PR, which is the common logic shared by preflight check & node controller, thanks. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
sys/sys.go (1)
183-204: Improve error messages for better debuggingWhile the function handles errors well, more detailed error messages would aid in debugging.
Apply this diff:
func parseKernelModuleConfigMap(contentReader io.Reader) (map[string]string, error) { configMap := map[string]string{} scanner := bufio.NewScanner(contentReader) for scanner.Scan() { config := scanner.Text() if !strings.HasPrefix(config, "CONFIG_") { continue } key, val, parsable := strings.Cut(config, "=") if !parsable { - return nil, fmt.Errorf("failed to parse kernel config %s", config) + return nil, fmt.Errorf("invalid kernel config format (missing '='): %s", config) } + if strings.TrimSpace(key) == "" || strings.TrimSpace(val) == "" { + return nil, fmt.Errorf("invalid kernel config (empty key or value): %s", config) + } configMap[strings.TrimSpace(key)] = strings.TrimSpace(val) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 201-202: sys/sys.go#L201-L202
Added lines #L201 - L202 were not covered by testssys/sys_test.go (2)
159-184: Enhance test coverage with additional test cases.The current test cases could be expanded to cover more edge cases.
Consider adding these test cases:
testCases := map[string]testCase{ // ... existing test cases ... + "GetBootKernelConfigMap(...): malformed line format": { + mockFileContent: "CONFIG_OPTION", // missing '=' separator + expectedConfigMap: nil, + expectedError: true, + }, + "GetBootKernelConfigMap(...): mixed valid and invalid lines": { + mockFileContent: "CONFIG_VALID=y\nINVALID\nCONFIG_ALSO_VALID=m", + expectedConfigMap: nil, + expectedError: true, + }, + "GetBootKernelConfigMap(...): whitespace handling": { + mockFileContent: " CONFIG_SPACE=y \n\t CONFIG_TAB=m\n\n", + expectedConfigMap: map[string]string{ + "CONFIG_SPACE": "y", + "CONFIG_TAB": "m", + }, + expectedError: false, + }, }
258-269: Enhance real config file test case.The test case for the real config file could include more specific checks.
Consider enhancing the real config test case:
if _, err := os.ReadFile(realConfigPath); err == nil { testCases["GetProcKernelConfigMap(...): real config"] = testCase{ setup: func(dir string) { err := io.CopyFile(realConfigPath, filepath.Join(dir, "config.gz"), true) c.Assert(err, IsNil) }, expectedConfigMapChecker: func(cm map[string]string) bool { - return len(cm) > 0 + // Check for common kernel config options that should exist + requiredKeys := []string{ + "CONFIG_64BIT", + "CONFIG_X86_64", + "CONFIG_MODULES", + } + for _, key := range requiredKeys { + if _, exists := cm[key]; !exists { + return false + } + } + return len(cm) > len(requiredKeys) }, expectedError: false, } }nfs/nfs.go (5)
24-25: Add test case for default config path scenario.The case where
configPrefixis empty and defaults to"/etc"is not covered by tests. Including a test case for this scenario ensures that the default behavior functions as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-25: nfs/nfs.go#L24-L25
Added lines #L24 - L25 were not covered by tests
31-32: Improve test coverage for error handling when reading configuration.The error path where
getSystemNFSMountConfigMapreturns an error other thanos.ErrNotExistis not covered by tests. Adding a test case wherenfsmount.confcannot be read due to permission issues or corruption would enhance robustness.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-32: nfs/nfs.go#L31-L32
Added lines #L31 - L32 were not covered by tests
41-42: Add tests for invalid minor version parsing.The scenario where the minor version in
Defaultversis invalid is not currently tested. Including a test case whereDefaultverscontains an invalid minor version (e.g.,"4.x") will ensure error handling is effective.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-42: nfs/nfs.go#L41-L42
Added lines #L41 - L42 were not covered by tests
68-68: Ensure empty lines are correctly handled in tests.The code skips empty lines in the configuration file, but this behavior isn't covered by tests. Adding a test case with empty lines in
nfsmount.confverifies that the parser handles them appropriately.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-68: nfs/nfs.go#L68
Added line #L68 was not covered by tests
86-87: Add test case for scanner errors to enhance error handling.The error check for
scanner.Err()is not covered by tests. Introducing a test case that triggers a scanner error, such as a line exceeding the buffer size, will ensure that this error path is tested.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-87: nfs/nfs.go#L86-L87
Added lines #L86 - L87 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
nfs/nfs.go(1 hunks)nfs/nfs_test.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(2 hunks)types/sys.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- types/sys.go
🚧 Files skipped from review as they are similar to previous changes (1)
- nfs/nfs_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
nfs/nfs.go
[warning] 24-25: nfs/nfs.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 31-32: nfs/nfs.go#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 41-42: nfs/nfs.go#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 68-68: nfs/nfs.go#L68
Added line #L68 was not covered by tests
[warning] 81-82: nfs/nfs.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 86-87: nfs/nfs.go#L86-L87
Added lines #L86 - L87 were not covered by tests
sys/sys.go
[warning] 139-140: sys/sys.go#L139-L140
Added lines #L139 - L140 were not covered by tests
[warning] 142-143: sys/sys.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 152-153: sys/sys.go#L152-L153
Added lines #L152 - L153 were not covered by tests
[warning] 162-163: sys/sys.go#L162-L163
Added lines #L162 - L163 were not covered by tests
[warning] 172-173: sys/sys.go#L172-L173
Added lines #L172 - L173 were not covered by tests
[warning] 177-178: sys/sys.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 201-202: sys/sys.go#L201-L202
Added lines #L201 - L202 were not covered by tests
🔇 Additional comments (6)
sys/sys.go (3)
4-7: LGTM: Import additions are appropriate
The new imports support the required functionality for file reading, compression handling, and I/O operations.
Also applies to: 17-18
135-155:
Add path traversal protection
While the function handles basic validation, it needs additional security measures:
- Sanitize kernelVersion to prevent path traversal attacks
- Validate bootDir path
Apply this diff to enhance security:
func GetBootKernelConfigMap(bootDir, kernelVersion string) (configMap map[string]string, err error) {
if kernelVersion == "" {
return nil, fmt.Errorf("kernelVersion cannot be empty")
}
+ if strings.Contains(kernelVersion, "..") || strings.Contains(kernelVersion, "/") {
+ return nil, fmt.Errorf("invalid kernelVersion: must not contain path traversal sequences")
+ }
if bootDir == "" {
bootDir = types.SysBootDirectory
}
+ if strings.Contains(bootDir, "..") {
+ return nil, fmt.Errorf("invalid bootDir: must not contain path traversal sequences")
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-140: sys/sys.go#L139-L140
Added lines #L139 - L140 were not covered by tests
[warning] 142-143: sys/sys.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 152-153: sys/sys.go#L152-L153
Added lines #L152 - L153 were not covered by tests
137-204: Improve test coverage for error conditions
The static analysis indicates several uncovered code paths, particularly around error handling. Consider adding test cases for:
- Empty or invalid kernelVersion/bootDir/procDir
- File open/read errors
- Malformed config entries
- Gzip reader errors
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for these scenarios?
✅ Verification successful
Test coverage is already comprehensive
The test file sys/sys_test.go already includes thorough test coverage for both GetBootKernelConfigMap and GetProcKernelConfigMap functions, covering:
- Empty kernelVersion/bootDir validation
- File read errors (implicitly through non-existent paths)
- Malformed config entries (test case "invalid content")
- Empty config files
- Successful parsing of various config formats
- Gzip handling for proc config
The test cases include both positive and negative scenarios, with proper error checking and validation of the returned maps. No additional test coverage is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the new functions
# Look for existing test files
fd -e go -e _test.go$ . | grep -l "TestGetBootKernelConfigMap\|TestGetProcKernelConfigMap\|TestParseKernelModuleConfigMap"
# Check for existing test cases
rg -A 5 "func Test(GetBootKernelConfigMap|GetProcKernelConfigMap|ParseKernelModuleConfigMap)"
Length of output: 222
Script:
#!/bin/bash
# Let's try a different approach to find test files and coverage
# First, find all test files in the sys directory since the code is in sys/sys.go
fd -e go -e _test.go$ sys/
# Then look for any test functions related to kernel config in these files
rg -l "kernel.*config" sys/*_test.go
# Finally, let's look at the actual test content
rg -A 10 "func Test.*[Kk]ernel.*[Cc]onfig" sys/
Length of output: 459
Script:
#!/bin/bash
# Let's correct the path issues and look at the test content
# Find test files
fd -e go -e _test.go$ . --full-path
# Look at the content of sys_test.go since we found it contains kernel config tests
cat sys/sys_test.go
# Also search for any mocks or test utilities that might be used
rg -l "Mock.*[Kk]ernel" .
Length of output: 106001
Script:
#!/bin/bash
# Let's try a simpler approach to find and examine test files
# First, look at the content of sys_test.go directly
cat sys/sys_test.go
# Also check for any test files in parent directories
find . -maxdepth 2 -type f -name "*test.go" -exec grep -l "Test.*[Kk]ernel.*[Cc]onfig" {} \;
Length of output: 8254
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-140: sys/sys.go#L139-L140
Added lines #L139 - L140 were not covered by tests
[warning] 142-143: sys/sys.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 152-153: sys/sys.go#L152-L153
Added lines #L152 - L153 were not covered by tests
[warning] 162-163: sys/sys.go#L162-L163
Added lines #L162 - L163 were not covered by tests
[warning] 172-173: sys/sys.go#L172-L173
Added lines #L172 - L173 were not covered by tests
[warning] 177-178: sys/sys.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 201-202: sys/sys.go#L201-L202
Added lines #L201 - L202 were not covered by tests
sys/sys_test.go (1)
192-193: Add error handling for file operations.
The error from os.WriteFile should be checked to ensure the test setup is successful.
Apply this diff to add proper error handling:
-err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
-c.Assert(err, IsNil)
+err := os.WriteFile(filepath.Join(bootDir, "config-"+kernelVersion), []byte(testCase.mockFileContent), 0644)
+c.Assert(err, IsNil, Commentf("Failed to write test file: %v", err))nfs/nfs.go (2)
3-12: Nitpick: Reorganize imports to match Go conventions.
According to Go conventions, imports should be grouped with standard library packages first, followed by third-party packages, separated by a blank line. Reorganizing the imports enhances readability.
Apply this diff to reorder the imports:
import (
"bufio"
"fmt"
"os"
"path/filepath"
"strconv"
"strings"
+ "github.com/pkg/errors"
- "github.com/pkg/errors"
)81-82: Consider skipping invalid key-value pairs instead of failing.
Currently, the parser returns an error if it encounters an invalid key-value pair. This could cause issues if the configuration file contains unintended lines or formatting errors. Consider logging a warning and skipping these lines to make the parser more resilient.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-82: nfs/nfs.go#L81-L82
Added lines #L81 - L82 were not covered by tests
Signed-off-by: Raphanus Lo <[email protected]>
COLDTURNIP
force-pushed
the
9830-check_nfs_version_in_nfsmount
branch
from
12acdb8
to
b4e05aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
sys/sys.go (1)
183-204: Add input validation for contentReaderThe function should validate the input reader before processing.
func parseKernelModuleConfigMap(contentReader io.Reader) (map[string]string, error) { + if contentReader == nil { + return nil, fmt.Errorf("contentReader cannot be nil") + } configMap := map[string]string{}sys/sys_test.go (4)
192-193: Improve error handling for file operations.The error handling for
os.WriteFilecould be more descriptive by including the test case name in the error message.-c.Assert(err, IsNil) +c.Assert(err, IsNil, Commentf("Failed to write test file for %v: %v", testName, err))
196-201: Improve test assertions order and error messages.The error assertions should be checked before comparing the results, and error messages could be more descriptive.
-c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf(test.ErrResultFmt, testName)) if testCase.expectedError { - c.Assert(err, NotNil, Commentf(test.ErrErrorFmt, testName, err)) + c.Assert(err, NotNil, Commentf("Expected error for test case %v", testName)) + c.Assert(exact, IsNil, Commentf("Expected nil result for error case %v", testName)) } else { - c.Assert(err, IsNil) + c.Assert(err, IsNil, Commentf("Unexpected error for test case %v: %v", testName, err)) + c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf("Unexpected result for test case %v", testName)) }
258-269: Enhance real config test case.The real config test case could be improved in several ways:
- The check for config existence could be more descriptive
- The expectedConfigMapChecker is too permissive, only checking for non-empty map
if _, err := os.ReadFile(realConfigPath); err == nil { + // Add real config test case only if the system has a valid kernel config testCases["GetProcKernelConfigMap(...): real config"] = testCase{ setup: func(dir string) { err := io.CopyFile(realConfigPath, filepath.Join(dir, "config.gz"), true) c.Assert(err, IsNil) }, expectedConfigMapChecker: func(cm map[string]string) bool { - return len(cm) > 0 + // Check for common kernel config entries that should exist + requiredConfigs := []string{"CONFIG_64BIT", "CONFIG_X86_64"} + for _, cfg := range requiredConfigs { + if _, exists := cm[cfg]; !exists { + return false + } + } + return len(cm) > len(requiredConfigs) }, expectedError: false, } }
276-286: Improve error handling and assertions.The error handling and assertions could be more descriptive and better organized.
exact, err := GetProcKernelConfigMap(procDir) +if testCase.expectedError { + c.Assert(err, NotNil, Commentf("Expected error for test case %v", testName)) + c.Assert(exact, IsNil, Commentf("Expected nil result for error case %v", testName)) +} else { + c.Assert(err, IsNil, Commentf("Unexpected error for test case %v: %v", testName, err)) if testCase.expectedConfigMapChecker == nil { - c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf(test.ErrResultFmt, testName)) + c.Assert(exact, DeepEquals, testCase.expectedConfigMap, Commentf("Unexpected result for test case %v", testName)) } else { - c.Assert(testCase.expectedConfigMapChecker(exact), Equals, true, Commentf(test.ErrResultFmt, testName)) + c.Assert(testCase.expectedConfigMapChecker(exact), Equals, true, Commentf("Config map checker failed for test case %v", testName)) } - if testCase.expectedError { - c.Assert(err, NotNil, Commentf(test.ErrErrorFmt, testName, err)) - } else { - c.Assert(err, IsNil) - } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
nfs/nfs.go(1 hunks)nfs/nfs_test.go(1 hunks)sys/sys.go(3 hunks)sys/sys_test.go(2 hunks)types/sys.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- nfs/nfs.go
- nfs/nfs_test.go
- types/sys.go
🔇 Additional comments (6)
sys/sys.go (4)
Line range hint 4-18: LGTM: Import changes are appropriate
The new imports are properly organized and necessary for the added functionality.
157-181: Enhance error handling with descriptive messages
The error messages should provide more context about the failure points.
configPath := filepath.Join(procDir, "config.gz")
configFile, err := os.Open(configPath)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to open config file %s: %w", configPath, err)
}
defer configFile.Close()
gzReader, err := gzip.NewReader(configFile)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to create gzip reader for %s: %w", configPath, err)
}135-155: 🛠️ Refactor suggestion
Add path traversal protection
While the function handles basic validation, it needs protection against path traversal in bootDir.
func GetBootKernelConfigMap(bootDir, kernelVersion string) (configMap map[string]string, err error) {
if kernelVersion == "" {
return nil, fmt.Errorf("kernelVersion cannot be empty")
}
+ if strings.Contains(bootDir, "..") {
+ return nil, fmt.Errorf("bootDir must not contain path traversal sequences")
+ }
if bootDir == "" {
bootDir = types.SysBootDirectory
}Likely invalid or redundant comment.
157-181: Improve test coverage for error handling
The following lines need test coverage:
- Lines 162-163: Default procDir assignment
- Lines 172-173: File open error handling
- Lines 177-178: Gzip reader error handling
Please add test cases for these error scenarios.
sys/sys_test.go (2)
186-188: Add cleanup for test files between test cases.
This matches a previous review comment about adding cleanup between test cases to prevent test pollution.
205-216: Improve resource management in helper function.
This matches a previous review comment about improving resource management in the genKernelConfig helper function.
|
|
||
| configMap, err := getSystemNFSMountConfigMap(configDir) | ||
| if os.IsNotExist(err) { | ||
| return defaultMajor, defaultMinor, nil |
There was a problem hiding this comment.
Just curious, what is the reason for returning the default major and minor instead of an error?
There was a problem hiding this comment.
If nfsmount.conf is absent, the NFS client actually negotiates with the server from the latest supported version, and it would be 4 currently. Caller should be aware of default protocol version if we return an error here.
There was a problem hiding this comment.
To make it simpler, will remove the default versions from parameter. Caller is responsible to handle the configuration absence.
| // getSystemNFSMountConfigMap reads the nfsmount.conf under configDir. The returned result is in | ||
| // map[section]map[key]value structure, and the global options is result["NFSMount_Global_Options"]. Refer to man page | ||
| // for more detail: man 5 nfsmount.conf | ||
| func getSystemNFSMountConfigMap(configPrefix string) (map[string]map[string]string, error) { |
There was a problem hiding this comment.
| func getSystemNFSMountConfigMap(configPrefix string) (map[string]map[string]string, error) { | |
| func getSystemNFSMountConfigMap(configDir string) (map[string]map[string]string, error) { |
| @@ -2,6 +2,9 @@ package types | |||
|
|
|||
| const OsReleaseFilePath = "/etc/os-release" | |||
| const SysClassBlockDirectory = "/sys/class/block/" | |||
| const SysBootDirectory = "/boot/" | |||
| const SysProcDirectory = "/proc/" | |||
| const SysConfigDirectory = "/etc/" | |||
There was a problem hiding this comment.
| const SysConfigDirectory = "/etc/" | |
| const SysEtcDirectory = "/etc/" |
| // map[section]map[key]value structure, and the global options is result["NFSMount_Global_Options"]. Refer to man page | ||
| // for more detail: man 5 nfsmount.conf | ||
| func getSystemNFSMountConfigMap(configPrefix string) (map[string]map[string]string, error) { | ||
| configFilePath := filepath.Join(configPrefix, "nfsmount.conf") |
There was a problem hiding this comment.
It seems nfsmount.conf is being used in multiple places. Could we condition having it in types.go?
| err = errors.Wrap(err, "failed to get kernel config map") | ||
| }() | ||
|
|
||
| configPath := filepath.Join(procDir, "config.gz") |
There was a problem hiding this comment.
Same here, could we consider moving config.gz to types.go?
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9830
What this PR does / why we need it:
We need to check NFS version information both in preflight checker and the node controller in LH manager. This PR provide the common logic for the above checks.
Special notes for your reviewer:
Additional documentation or context