SKIP-1159 - A better labeler #557
Conversation
- Fixed most cases
…e robust - All tests pass now
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Tool Failures:Tool Failure Count:Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
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
CodeRabbit Configuration File (
|
Co-authored-by: Martin Haram Nygård <martin.nygard@kartverket.no>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/resourcegenerator/resourceutils/helpers_test.go (1)
9-39: Consider adding negative test cases.While the current test cases are comprehensive for valid inputs, consider adding negative test cases to verify proper handling of:
- Invalid image strings
- Malformed version strings
- Empty strings
- Very long version strings that exceed LabelValueMaxLength
This would further improve the robustness of the labeler.
pkg/resourcegenerator/resourceutils/helpers.go (3)
24-28: Handle errors explicitly inMatchesRegexfunctionCurrently, the function checks
err == nilin the return statement. For better clarity and error handling, consider checking iferris notnilbefore returning the result. This helps in debugging and maintaining the code.Apply this diff to handle errors explicitly:
func MatchesRegex(s string, pattern string) bool { obj, err := regexp.Match(pattern, []byte(s)) - return obj && err == nil + if err != nil { + // Handle or log the error as needed + return false + } + return obj }
33-36: Ensure consistent handling of image digestsWhen an image string contains an
"@", the code replaces everything after it with":unknown". This could lead to confusion or mislabeling. Consider using a more descriptive tag or handling the digest appropriately.For example, you might retain the digest information or use a more meaningful placeholder:
if strings.Contains(imageVersionString, "@") { imageVersionString = strings.Split(imageVersionString, "@")[0] - imageVersionString = imageVersionString + ":unknown" + imageVersionString = imageVersionString + ":digest" }
57-58: Prevent potential issues in version string sanitization loopThe loop that removes leading characters not matching
[a-zA-Z0-9]may result in an emptyversionPart, which could cause issues downstream if an empty string is not an acceptable version label. Consider adding a check to handle this scenario.For example:
for len(versionPart) > 0 && !MatchesRegex(versionPart[:1], "[a-zA-Z0-9]") { versionPart = versionPart[1:] } + if len(versionPart) == 0 { + versionPart = "unknown" + }This ensures that
versionPartwill have a valid value even if all leading characters were removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/resourcegenerator/resourceutils/helpers.go(2 hunks)pkg/resourcegenerator/resourceutils/helpers_test.go(1 hunks)
🔇 Additional comments (8)
pkg/resourcegenerator/resourceutils/helpers_test.go (3)
1-7: LGTM! Clean import section.
The import section is well-organized with only the necessary testing dependency.
9-13: Excellent use of table-driven tests!
The test structure follows Go best practices, making it easy to maintain and extend with new test cases.
34-39: Well-structured test execution!
The use of subtests with descriptive names and clear assertions will make test failures easy to diagnose.
pkg/resourcegenerator/resourceutils/helpers.go (5)
6-6: Approved: Necessary imports added
The addition of "regexp" and "strings" imports is appropriate and necessary for the functions introduced later in the code.
10-11: Approved: Define LabelValueMaxLength constant
Defining LabelValueMaxLength as 63 correctly sets the maximum length for Kubernetes label values, adhering to Kubernetes specifications.
39-41: Approved: Default to "latest" when no version is specified
The logic correctly assumes "latest" when no version tag is present in the image string.
49-49: Approved: Replace "+" with "-" in version strings
Replacing "+" with "-" in the version string helps maintain compatibility with label value requirements.
52-53: Approved: Enforce maximum label value length
Truncating the versionPart to LabelValueMaxLength characters ensures compliance with Kubernetes label length constraints.
| {"image", "latest"}, | ||
| {"image:latest", "latest"}, | ||
| {"image:1.2.3-dev-123abc", "1.2.3-dev-123abc"}, | ||
| {"image:1.2.3", "1.2.3"}, | ||
| {"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "unknown"}, | ||
| {"ghcr.io/org/one-app:sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4"}, | ||
| {"ghcr.io/org/another-app:3fb7048", "3fb7048"}, | ||
| {"ghcr.io/org/some-team/third-app:v1.2.54", "v1.2.54"}, | ||
| {"ghcr.io/org/another-team/fourth-app:4.0.0.rc-36", "4.0.0.rc-36"}, | ||
| {"ghcr.io/org/another-team/fifth-app:4.0.0.rc-36-master-latest", "4.0.0.rc-36-master-latest"}, | ||
| {"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "unknown"}, | ||
| {"ghcr.io/kartverket/vulnerability-disclosure-program:4.0.1@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "4.0.1"}, | ||
| {"nginxinc/nginx-unprivileged:1.20.0-alpine", "1.20.0-alpine"}, | ||
| {"foo/bar:1.2.3+build.4", "1.2.3-build.4"}, | ||
| {"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"}, | ||
| {"foo/bar:-1.2.3", "1.2.3"}, | ||
| {"foo/bar:__1.2.3", "1.2.3"}, | ||
| {"foo/bar:.1.2.3", "1.2.3"}, | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needs improvement for version string transformations
The implementation in GetImageVersion includes several important transformations that should be better documented in the function's header comment:
- Handling of SHA digests (returns "unknown")
- Replacing "+" with "-" in version strings
- Truncating version strings longer than
LabelValueMaxLength - Stripping non-alphanumeric leading characters
- Default "latest" tag handling
The current comment // GetImageVersion returns the version part of an image string is too brief given the complexity of the transformations.
🔗 Analysis chain
Comprehensive test coverage with clear test cases!
The test cases cover a wide range of scenarios including:
- Basic image tags
- Semantic versions
- SHA digests
- Special character handling
- Version string transformations
Let's verify if these transformations are documented in the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the version string transformation rules are documented
# Expected: Comments or documentation explaining the transformation rules
# Look for documentation about version transformations
rg -A 5 "GetImageVersion" pkg/resourcegenerator/resourceutils/helpers.go
Length of output: 429
Script:
#!/bin/bash
# Let's see the complete implementation of GetImageVersion to understand all transformations
rg -A 20 "GetImageVersion" pkg/resourcegenerator/resourceutils/helpers.go
# Also check for any related tests that might document the behavior
rg -A 10 "TestGetImageVersion" pkg/resourcegenerator/resourceutils/helpers_test.go
Length of output: 901
Script:
#!/bin/bash
# Let's try with ast-grep to get the complete function implementation
ast-grep --pattern 'func GetImageVersion($_) string {
$$$
}'
# And get the test function as well
ast-grep --pattern 'func TestGetImageVersion($_) $_ {
$$$
}'
Length of output: 2719
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
pkg/resourcegenerator/resourceutils/helpers.go (1)
30-30: Add comprehensive function documentationThe function should document its behavior for different input formats and edge cases:
- Image strings with digests (@sha256:...)
- Images without tags
- Version sanitization rules
Add this documentation:
-// GetImageVersion returns the version part of an image string +// GetImageVersion extracts and sanitizes the version tag from an image string. +// It handles various formats: +// - Images with digests (e.g., image@sha256:123) -> returns "unknown" +// - Images without tags (e.g., nginx) -> returns "latest" +// - Images with tags (e.g., nginx:1.19) -> returns sanitized tag +// The returned tag is sanitized to be valid as a Kubernetes label value: +// - Maximum length of 63 characters +// - Must start with alphanumeric character +// - '+' characters replaced with '-'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/resourcegenerator/resourceutils/helpers.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Build and run tests
pkg/resourcegenerator/resourceutils/helpers.go
[failure] 57-57:
undefined: MatchesRegex
🔇 Additional comments (3)
pkg/resourcegenerator/resourceutils/helpers.go (3)
6-6: LGTM! Follows Kubernetes conventions
The LabelValueMaxLength constant of 63 correctly adheres to Kubernetes label value length limitations.
Also applies to: 10-10
30-61: Verify usage and test coverage
Let's ensure all callers handle the new edge cases (empty string -> "unknown") and that test coverage is adequate.
✅ Verification successful
The previous output showed that GetImageVersion is used in deployment, service, and job generators to set the version label. We need to see the test coverage to ensure edge cases are handled properly, especially for the "@" handling and empty string cases.
Test coverage is comprehensive and implementation is correct
The test coverage is thorough with test cases covering all edge cases including:
- Images with SHA digests (both with and without tags)
- Empty versions defaulting to "latest"
- Version strings with "+" being converted to "-"
- Long version strings being truncated
- Version strings with invalid starting characters
- Various real-world version formats
The implementation correctly handles all these cases and is being used consistently across deployment, service, and job generators to set the version label. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of GetImageVersion
echo "=== GetImageVersion usage ==="
rg "GetImageVersion\(" -A 2
# Check test coverage for edge cases
echo -e "\n=== Test coverage ==="
rg "TestVersion" -A 10 ".*_test\.go$"
Length of output: 1873
Script:
#!/bin/bash
# Let's check the test file content specifically
echo "=== Test content ==="
cat pkg/resourcegenerator/resourceutils/helpers_test.go
# Let's also check the actual implementation to compare with tests
echo -e "\n=== Implementation ==="
cat pkg/resourcegenerator/resourceutils/helpers.go
Length of output: 3786
🧰 Tools
🪛 GitHub Check: Build and run tests
[failure] 57-57:
undefined: MatchesRegex
43-45:
Fix handling of image strings with multiple colons
The current implementation can break with registry URLs containing ports (e.g., "registry:5000/image:tag").
Apply this diff:
- // Split image string into parts
- parts := strings.Split(imageVersionString, ":")
-
- versionPart := parts[1]
+ // Extract version using last colon
+ lastColon := strings.LastIndex(imageVersionString, ":")
+ if lastColon == -1 || lastColon == len(imageVersionString)-1 {
+ return "latest"
+ }
+ versionPart := imageVersionString[lastColon+1:]Likely invalid or redundant comment.
There was a problem hiding this comment.
The more special cases I think of the more I think that the return type should be (string, error). What do you think @martinhny @omaen @BardOve?
Depends on how important the label is for further use. I feel like the label is kind of soft/best effort, but it would be nice to actually be able to identify errors if they do happen. Should we in that case log the error in skiperator stdout or create a condition for it? |
|
I'm for logging a WARN since it's a best effort thing done mainly for ourselves |
- Refactored function-name to better reflect what it does
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
pkg/resourcegenerator/resourceutils/helpers.go (3)
27-29: Move constant declaration to package level.The
LabelValueMaxLengthconstant should be declared at package level since it's a fixed value that could be reused across the package.Apply this diff:
package resourceutils import ( ... ) +const LabelValueMaxLength = 63 + // HumanReadableVersion returns the version part of an image string func HumanReadableVersion(imageReference string) string { - const LabelValueMaxLength = 63
45-48: Simplify prefix trimming logic using strings.TrimLeftFunc.The current implementation using a loop can be simplified using
strings.TrimLeftFunc.Apply this diff:
- // While first character is not part of regex [a-z0-9A-Z] then remove it - for len(versionPart) > 0 && !matchesRegex(versionPart[:1], "[a-zA-Z0-9]") { - versionPart = versionPart[1:] - } + // Trim non-alphanumeric prefix + versionPart = strings.TrimLeftFunc(versionPart, func(r rune) bool { + return !alphanumericPattern.MatchString(string(r)) + })
50-59: Consider using strings.Map for character replacement.The current implementation using strings.Builder could be simplified using strings.Map.
Apply this diff:
- // For each character in versionPart, replace characters that are not allowed in label-value - var result strings.Builder - for _, c := range versionPart { - if allowedChars.MatchString(string(c)) { - result.WriteRune(c) - } else { - result.WriteRune('-') - } - } - versionPart = result.String() + // Replace characters that are not allowed in label-value + versionPart = strings.Map(func(r rune) rune { + if allowedChars.MatchString(string(r)) { + return r + } + return '-' + }, versionPart)pkg/resourcegenerator/job/job.go (1)
Line range hint
142-147: Remove commented out code.Since this functionality has been replaced with
resourceutils.HumanReadableVersion, the commented-out function should be removed to maintain code cleanliness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
pkg/resourcegenerator/deployment/deployment.go(1 hunks)pkg/resourcegenerator/job/job.go(1 hunks)pkg/resourcegenerator/resourceutils/helpers.go(2 hunks)pkg/resourcegenerator/resourceutils/helpers_test.go(1 hunks)pkg/resourcegenerator/service/service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/resourcegenerator/resourceutils/helpers_test.go
🔇 Additional comments (4)
pkg/resourcegenerator/resourceutils/helpers.go (2)
10-13: 🛠️ Refactor suggestion
Improve regex pattern handling efficiency and error handling.
The current implementation recompiles the regex pattern on every call and masks potential regexp compilation errors. Consider using regexp.MustCompile at package level for frequently used patterns.
Apply this diff:
-func matchesRegex(s string, pattern string) bool {
- obj, err := regexp.Match(pattern, []byte(s))
- return obj && err == nil
+var alphanumericPattern = regexp.MustCompile(`[a-zA-Z0-9]`)
+
+func matchesRegex(s string) bool {
+ return alphanumericPattern.MatchString(s)
}Likely invalid or redundant comment.
33-36: Consider handling malformed digest format.
The current implementation assumes the digest format is valid. Consider handling cases where multiple "@" symbols might exist.
pkg/resourcegenerator/service/service.go (1)
40-40: LGTM! Verify consistent usage across resources.
The change to use HumanReadableVersion aligns with the PR's goal of improving the labeler's robustness.
Let's verify that this change is consistently applied across all resource generators:
Note: Based on the PR discussion about error handling, consider adding warning-level logging in HumanReadableVersion for cases where version extraction fails, rather than silently falling back to defaults.
✅ Verification successful
Version labeling is consistently implemented across resources
The verification confirms that HumanReadableVersion is consistently used for version labels in all relevant resource generators:
- Service generator:
service.Labels["app.kubernetes.io/version"] - Job generator:
labels["app.kubernetes.io/version"] - Deployment generator:
podTemplateLabels["app.kubernetes.io/version"]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent version label implementation across resource generators
# Expected: All resource generators should use HumanReadableVersion for version labels
# Test: Search for version label implementations
rg -A 2 "app.kubernetes.io/version" pkg/resourcegenerator/
Length of output: 928
pkg/resourcegenerator/job/job.go (1)
148-148: Consider error handling for version extraction.
Based on the PR discussion about error handling, consider logging warnings when version extraction fails or returns unexpected results. This aligns with the consensus in the PR comments about using warning logs for error cases.
Let's verify the error handling in the new HumanReadableVersion function:
| // Limit label-value to 63 characters | ||
| if len(versionPart) > LabelValueMaxLength { | ||
| versionPart = versionPart[:LabelValueMaxLength] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty version after processing.
The function should handle cases where the version might become empty after all the processing.
Apply this diff:
if len(versionPart) > LabelValueMaxLength {
versionPart = versionPart[:LabelValueMaxLength]
}
+
+ // Return "unknown" if version is empty after processing
+ if len(versionPart) == 0 {
+ return "unknown"
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -108,7 +108,7 @@ func Generate(r reconciliation.Reconciliation) error { | |||
| } else { | |||
| podTemplateLabels = util.GetPodAppSelector(application.Name) | |||
| } | |||
| podTemplateLabels["app.kubernetes.io/version"] = resourceutils.GetImageVersion(application.Spec.Image) | |||
| podTemplateLabels["app.kubernetes.io/version"] = resourceutils.HumanReadableVersion(application.Spec.Image) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent version labeling found in job generator
The verification revealed that GetImageVersion is still being used in pkg/resourcegenerator/job/job.go while other resource generators have been updated to use HumanReadableVersion. This inconsistency needs to be addressed.
pkg/resourcegenerator/job/job.go: Contains bothGetImageVersionandHumanReadableVersionusage- Other generators correctly use
HumanReadableVersion:pkg/resourcegenerator/service/service.gopkg/resourcegenerator/deployment/deployment.go
🔗 Analysis chain
LGTM! Consider adding error logging for labeling failures.
The change to use HumanReadableVersion aligns with the PR's goal of improving labeling robustness and maintains consistency across deployment, job, and service resources.
Let's verify the consistent implementation across other files:
Based on the PR discussion, consider adding error logging when the version label cannot be generated correctly, as this is a best-effort feature. This would help with debugging while maintaining the non-blocking nature of the labeling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of HumanReadableVersion across resource generators
# Test: Check if GetImageVersion is still used anywhere
echo "Checking for any remaining usage of GetImageVersion..."
rg "GetImageVersion\(" pkg/resourcegenerator/
# Test: Verify HumanReadableVersion is used consistently
echo "Verifying HumanReadableVersion usage..."
rg "HumanReadableVersion\(" pkg/resourcegenerator/
Length of output: 1064
Redid logic to make it more robust
Summary by CodeRabbit
New Features
Bug Fixes
Tests