Fix CloudProfile API rejection by skipping invalid SemVer legacy tags#34
Conversation
|
Warning Review limit reached
More reviews will be available in 23 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesSemVer validation in image filtering and updating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloudprofilesync/imageupdater.go (1)
108-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing validation: CleanVersion may still be invalid when written.
If a
SourceImagehas a valid legacy tag but a truly unparseableCleanVersion(not even tolerantly parseable like"garbage"),filterImagesallows it through becausevalidLegacyTagis true. However,CleanVersionis not cleared—it remains the original invalid string. The condition on line 108 evaluates to true, causing the invalidCleanVersionto be written toimage.Versions, which would trigger the same API rejection this PR aims to fix.🐛 Proposed fix: validate CleanVersion before writing
// When capabilities are enabled, also write the clean version entry. if iu.EnableCapabilities && sourceImage.CleanVersion != "" && sourceImage.CleanVersion != sourceImage.Version { + // CleanVersion was normalized by filterImages if tolerantly parseable; + // skip writing if it's still invalid (truly unparseable input). + if _, err = semver.ParseTolerant(sourceImage.CleanVersion); err != nil { + iu.Log.V(1).Info("skipping clean version entry because it is not valid semver", "cleanVersion", sourceImage.CleanVersion) + continue + } if idx, exists := existingVersions[sourceImage.CleanVersion]; exists {Alternatively,
filterImagescould clearCleanVersionwhen parsing fails, but that may break downstream code expecting the original annotation value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/imageupdater.go` around lines 108 - 124, The code on line 108 checks if CleanVersion is non-empty but does not validate that it is actually a valid version string before writing it to image.Versions. Add validation to ensure CleanVersion is truly parseable and valid before using it in the condition that guards the append and update logic for image.Versions. If CleanVersion is found to be invalid during validation, skip this block entirely so invalid version strings are not written to the image.Versions array, preventing API rejection downstream.
🧹 Nitpick comments (1)
cloudprofilesync/imageupdater_test.go (1)
84-96: ⚡ Quick winTest name is misleading:
"1921.0"is tolerantly parseable, not truly invalid.The test name says "invalid clean version" but
"1921.0"is successfully parsed bysemver.ParseTolerantand normalized to"1921.0.0". This is more accurately described as "tolerantly parseable and normalized."A test for a truly unparseable
CleanVersion(e.g.,"garbage") is missing—this is the edge case that could expose the bug mentioned inimageupdater.gowhere an invalidCleanVersionmight be written to the spec.♻️ Suggested: rename test and add coverage for truly invalid CleanVersion
- It("valid tag + invalid clean version: BOTH formats with clean version normalized", func(ctx SpecContext) { + It("valid tag + tolerantly parseable clean version: BOTH formats with clean version normalized", func(ctx SpecContext) { result := versions(ctx, []cloudprofilesync.SourceImage{ { Version: "1921.0.0-metal-sci-usi-amd64", CleanVersion: "1921.0", Architectures: []string{"amd64"}, Capabilities: gardencorev1beta1.Capabilities{"architecture": {"amd64"}, "feature": {"sci", "_usi"}}, }, }) Expect(result).To(HaveLen(2)) versionStrings := []string{result[0].Version, result[1].Version} Expect(versionStrings).To(ContainElements("1921.0.0-metal-sci-usi-amd64", "1921.0.0")) }) + + It("valid tag + truly invalid clean version: OLD format only (clean version skipped)", func(ctx SpecContext) { + result := versions(ctx, []cloudprofilesync.SourceImage{ + { + Version: "1921.0.0-metal-sci-usi-amd64", + CleanVersion: "garbage", + Architectures: []string{"amd64"}, + }, + }) + Expect(result).To(HaveLen(1)) + Expect(result[0].Version).To(Equal("1921.0.0-metal-sci-usi-amd64")) + })Note: This new test will fail until the validation fix in
imageupdater.gois applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudprofilesync/imageupdater_test.go` around lines 84 - 96, The test named "valid tag + invalid clean version: BOTH formats with clean version normalized" is misleading because the CleanVersion value "1921.0" is actually tolerantly parseable by semver.ParseTolerant and gets normalized successfully—it is not truly invalid. Rename this test to accurately reflect that the CleanVersion is "tolerantly parseable and normalized" rather than "invalid," and then add a new separate test case that covers the actual edge case of a truly unparseable CleanVersion value (such as "garbage") to properly test the validation and error handling for invalid clean versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cloudprofilesync/imageupdater.go`:
- Around line 108-124: The code on line 108 checks if CleanVersion is non-empty
but does not validate that it is actually a valid version string before writing
it to image.Versions. Add validation to ensure CleanVersion is truly parseable
and valid before using it in the condition that guards the append and update
logic for image.Versions. If CleanVersion is found to be invalid during
validation, skip this block entirely so invalid version strings are not written
to the image.Versions array, preventing API rejection downstream.
---
Nitpick comments:
In `@cloudprofilesync/imageupdater_test.go`:
- Around line 84-96: The test named "valid tag + invalid clean version: BOTH
formats with clean version normalized" is misleading because the CleanVersion
value "1921.0" is actually tolerantly parseable by semver.ParseTolerant and gets
normalized successfully—it is not truly invalid. Rename this test to accurately
reflect that the CleanVersion is "tolerantly parseable and normalized" rather
than "invalid," and then add a new separate test case that covers the actual
edge case of a truly unparseable CleanVersion value (such as "garbage") to
properly test the validation and error handling for invalid clean versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8502fdb-b993-40c4-9091-d2981da6dc1e
📒 Files selected for processing (2)
cloudprofilesync/imageupdater.gocloudprofilesync/imageupdater_test.go
There was a problem hiding this comment.
Pull request overview
This PR addresses Gardener API rejections during CloudProfile synchronization by preventing invalid (non-SemVer) legacy OCI tags from being written into spec.machineImages[].versions[].version, while still allowing images to be processed via a validated/normalized CleanVersion when available (GEP-33 transition behavior).
Changes:
- Enhance
filterImages()to validate legacy tags and clean versions independently, normalize tolerantly-parseable clean versions, and drop entries where both are invalid. - Skip writing legacy
spec.machineImages[].versionsentries when the raw OCI tag is not valid SemVer, avoiding API server rejection. - Extend Ginkgo tests to cover filtering/dual-write scenarios, including invalid legacy tags with valid clean versions.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cloudprofilesync/imageupdater.go |
Validates legacy tag vs clean version separately; normalizes clean versions; skips writing invalid legacy SemVer into CloudProfile spec. |
cloudprofilesync/imageupdater_test.go |
Adds/extends tests for image filtering and legacy/clean version write behavior (flag ON/OFF). |
go.mod |
Pins Go version with patch level. |
.golangci.yaml |
Adjusts gomoddirectives Go version regex to allow patch versions. |
.typos.toml |
Adds an ignore-regex for a specific identifier (ANDed). |
.gitignore |
Ignores .claude directory. |
LICENSES/CC0-1.0.txt |
Removes CC0 license text file. |
.claude/settings.local.json |
Removes local Claude settings file from the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
With the recent GEP-33 updates,
cloud-profile-syncwas made smarter: it now correctly extracts a valid, clean semantic version (e.g.,1877.9.2) from OCI image annotations. Because this clean version passes the initialfilterImages()validation, images with historically malformed, non-SemVer compliant raw tags (e.g.,1877.9.2.0-metal-sci-pxe-amd64-...) are now successfully processed by the controllerHowever, when it attempts to write these raw tags back into the legacy
spec.machineImages[].versions[].versionarray for backward compatibility, the Gardener API Server strictly enforces SemVer validation. This caused the API server to reject the entire CloudProfile patch as Invalid, silently halting the synchronization processHow it works now with feature flag enabled:
Invalid SemVer Tag + Has CleanVersion: NEW format only.
Invalid SemVer Tag + Invalid/No CleanVersion: everything dropped
Valid SemVer Tag + Valid CleanVersion: BOTH formats (the ideal transition state).
Valid SemVer Tag + NO CleanVersion (Old images): OLD format only
Valid SemVer Tag + Invalid CleanVersion : BOTH formats (clean version will be adjusted to be valid)
Summary by CodeRabbit