Add feature flag for GEP-33 Capabilities support#30
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (2)
📝 WalkthroughWalkthroughThis PR adds optional machine image capability support: it parses OCI feature annotations into capabilities, exposes CleanVersion on source images, and when enabled writes both legacy full-tag entries and grouped clean-version CapabilityFlavors into the cloud profile; the feature is gated by a new CLI flag and reconciler field. ChangesMachine Image Capabilities Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cloudprofilesync/source_test.go (1)
159-195: ⚡ Quick winConsider adding test case for missing version annotation.
The current test validates the case where
feature_setcontains no valid values. However, there's no test covering the scenario wherefeature_sethas valid features but theversionannotation is missing. This edge case could occur in practice and should be tested to ensure proper error handling (related to the concern raised insource.golines 148-159).📝 Suggested test case
It("returns error when feature_set is valid but version annotation is missing", func(ctx SpecContext) { repo, err := remote.NewRepository(registryAddr + "/repo-no-version") Expect(err).To(Succeed()) repo.PlainHTTP = true index := ocispec.Index{ Versioned: specs.Versioned{SchemaVersion: 2}, Manifests: []ocispec.Descriptor{ {MediaType: ocispec.MediaTypeImageManifest, Size: 0, Digest: ocispec.DescriptorEmptyJSON.Digest}, }, Annotations: map[string]string{ "architecture": "amd64", "feature_set": "sci,_usi", // "version" annotation intentionally missing }, } indexBlob, err := json.Marshal(index) Expect(err).To(Succeed()) indexDesc := content.NewDescriptorFromBytes(ocispec.MediaTypeImageIndex, indexBlob) err = repo.Push(ctx, ocispec.DescriptorEmptyJSON, strings.NewReader("{}")) Expect(err).To(Succeed()) err = repo.PushReference(ctx, indexDesc, bytes.NewReader(indexBlob), "4.0.0-no-version") Expect(err).To(Succeed()) oci, err := cloudprofilesync.NewOCI(cloudprofilesync.OCIParams{ Registry: registryAddr, Repository: "repo-no-version", Parallel: 4, }, true) Expect(err).To(Succeed()) _, err = oci.GetVersions(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("version annotation not found")) })🤖 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/source_test.go` around lines 159 - 195, Add a test to cover the missing version annotation edge case: in source_test.go add an It block that creates a repo (e.g., "repo-no-version"), pushes an index whose Annotations include a valid "feature_set" (e.g., "sci,_usi") but omit the "version" key, then instantiate cloudprofilesync.NewOCI(...) and call oci.GetVersions(ctx) expecting an error (Expect(err).To(HaveOccurred()) and Expect(err.Error()).To(ContainSubstring("version annotation not found"))). This targets the logic in GetVersions/NewOCI that currently assumes a version annotation and verifies proper error handling when the "version" annotation is absent.cloudprofilesync/imageupdater_test.go (1)
130-191: 💤 Low valueConsider adding test for multiple flavors sharing the same CleanVersion.
The current tests validate dual-write for single images, but don't cover the scenario where multiple source images share the same
CleanVersion(e.g., different feature flavors of version "2254.0.0"). This would help verify that theArchitecturesfield and version entry creation logic works correctly when multiple flavors exist.📝 Suggested test case
It("handles multiple flavors with the same CleanVersion", func(ctx SpecContext) { mockSource.images = []cloudprofilesync.SourceImage{ { Version: "2254.0.0-baremetal-sci-usi-amd64", CleanVersion: "2254.0.0", Architectures: []string{"amd64"}, Capabilities: gardencorev1beta1.Capabilities{"architecture": {"amd64"}, "feature": {"sci", "_usi"}}, }, { Version: "2254.0.0-baremetal-sci-pxe-amd64", CleanVersion: "2254.0.0", Architectures: []string{"amd64"}, Capabilities: gardencorev1beta1.Capabilities{"architecture": {"amd64"}, "feature": {"sci", "_pxe"}}, }, } updater := cloudprofilesync.ImageUpdater{ Log: GinkgoLogr, Source: &mockSource, ImageName: "test", EnableCapabilities: true, } var cpSpec v1beta1.CloudProfileSpec Expect(updater.Update(ctx, &cpSpec)).To(Succeed()) // Should have 2 legacy entries + 1 clean version entry = 3 total Expect(cpSpec.MachineImages[0].Versions).To(HaveLen(3)) // Verify clean version entry exists versions := cpSpec.MachineImages[0].Versions cleanVersionCount := 0 for _, v := range versions { if v.Version == "2254.0.0" { cleanVersionCount++ } } Expect(cleanVersionCount).To(Equal(1), "Should only have one clean version entry") })🤖 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 130 - 191, Add a test that seeds mockSource.images with multiple cloudprofilesync.SourceImage entries that share the same CleanVersion but differ in Version/Capabilities (e.g., two flavors for "2254.0.0"), then call ImageUpdater.Update (using the existing updater setup with EnableCapabilities true) and assert that cpSpec.MachineImages[0].Versions contains entries for both full tags plus exactly one clean-version entry (assert total length == 3 and that the clean Version "2254.0.0" appears exactly once); place the test alongside the existing "flag ON (dual-write clean version)" cases to validate deduplication of CleanVersion while preserving Architectures/Capabilities for each flavor.cloudprofilesync/source.go (1)
21-29: 💤 Low valueConsider making the allowlist immutable or document why mutability is needed.
The
validFeatureValuesmap is declared as a package-levelvar, making it mutable throughout the package lifecycle. If this allowlist is intended to remain constant, consider one of:
- Declaring it as a
constmap (if Go version permits)- Documenting why mutability is necessary (e.g., for testing or runtime configuration)
- Using a function that returns the allowlist to prevent external modification
Additionally, the feature values lack documentation explaining their purpose (e.g., what "chost", "_pxe", "sci" represent in the context of GEP-33).
🤖 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/source.go` around lines 21 - 29, validFeatureValues is a package-level mutable map; make it effectively immutable and document the keys: replace the open var usage by keeping the literal data private but return copies via an accessor (e.g., implement a ValidFeatureValues() or GetValidFeatureValues() function that constructs/returns a fresh map or returns a read-only view) so callers cannot mutate the package allowlist, and add a short comment above the original validFeatureValues definition enumerating what each key (e.g., "chost", "_pxe", "sci", etc.) means in the context of GEP-33; this preserves current behavior while preventing accidental runtime modification.cloudprofilesync/imageupdater.go (1)
82-93: Clarify clean-versionArchitecturesrelevance under GEP-33
cloudprofilesync/imageupdater.gocreates the clean-versioncpSpec.MachineImages[].Versions[]entry only once and populates itsArchitecturesfrom the first source image for thatCleanVersion(so it can miss additional architectures across flavors). However, with GEP-33, image selection is capability-driven (CapabilityFlavorsin providerConfig) and the architecture fields are deprecated in favor of “architecture as capability”, so this incompleteness is likely not impactful for capability-based resolution.Optional: if Gardener’s mixed/legacy validation path still consults
Architectureson the clean-version entries, consider aggregating architectures across all flavors sharing the sameCleanVersionto avoid order-dependent results.🤖 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 82 - 93, The clean-version entry creation in the block guarded by iu.EnableCapabilities uses the Architectures from only the first sourceImage for a given sourceImage.CleanVersion, which can miss architectures from other flavors; update the logic where existingVersions[sourceImage.CleanVersion] is checked/created (affecting image.Versions and gardenerv1beta1.MachineImageVersion entries) to aggregate/union Architectures across all source images that share the same CleanVersion instead of leaving the slice as-is—i.e., when the clean-version already exists, merge any missing architectures from sourceImage.Architectures into image.Versions[existingIndex].Architectures, ensuring no duplicates.
🤖 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.
Inline comments:
In `@cloudprofilesync/source.go`:
- Around line 148-159: OCISource.GetVersions currently assigns Capabilities when
feature_set filters non-empty but unconditionally reads
manifest.Annotations["version"] into cleanVersion, which can leave CleanVersion
empty and cause downstream sinks (IroncoreProvider.Configure and
ImageUpdater.Update) to drop capability-flavor entries; fix by checking for the
presence of the "version" annotation before populating capabilities and
cleanVersion (only construct gardencorev1beta1.Capabilities and set cleanVersion
when manifest.Annotations has a non-empty "version"), and add a unit test
covering the "feature_set present but version missing" case to ensure
capabilities are not emitted silently.
In `@go.mod`:
- Around line 16-19: The go.mod currently pins k8s.io/api,
k8s.io/apiextensions-apiserver, k8s.io/apimachinery, and k8s.io/client-go at
v0.36.1 which mismatches sigs.k8s.io/controller-runtime v0.24.1 expectations
(v0.36.0); to fix, align versions by either downgrading the k8s.io/* entries to
v0.36.0 or upgrading controller-runtime to a release that expects v0.36.1, then
run go mod tidy and a build/test cycle to ensure the k8s.io/* set is consistent
and compilation succeeds (refer to module names k8s.io/api,
k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/client-go and
sigs.k8s.io/controller-runtime).
---
Nitpick comments:
In `@cloudprofilesync/imageupdater_test.go`:
- Around line 130-191: Add a test that seeds mockSource.images with multiple
cloudprofilesync.SourceImage entries that share the same CleanVersion but differ
in Version/Capabilities (e.g., two flavors for "2254.0.0"), then call
ImageUpdater.Update (using the existing updater setup with EnableCapabilities
true) and assert that cpSpec.MachineImages[0].Versions contains entries for both
full tags plus exactly one clean-version entry (assert total length == 3 and
that the clean Version "2254.0.0" appears exactly once); place the test
alongside the existing "flag ON (dual-write clean version)" cases to validate
deduplication of CleanVersion while preserving Architectures/Capabilities for
each flavor.
In `@cloudprofilesync/imageupdater.go`:
- Around line 82-93: The clean-version entry creation in the block guarded by
iu.EnableCapabilities uses the Architectures from only the first sourceImage for
a given sourceImage.CleanVersion, which can miss architectures from other
flavors; update the logic where existingVersions[sourceImage.CleanVersion] is
checked/created (affecting image.Versions and
gardenerv1beta1.MachineImageVersion entries) to aggregate/union Architectures
across all source images that share the same CleanVersion instead of leaving the
slice as-is—i.e., when the clean-version already exists, merge any missing
architectures from sourceImage.Architectures into
image.Versions[existingIndex].Architectures, ensuring no duplicates.
In `@cloudprofilesync/source_test.go`:
- Around line 159-195: Add a test to cover the missing version annotation edge
case: in source_test.go add an It block that creates a repo (e.g.,
"repo-no-version"), pushes an index whose Annotations include a valid
"feature_set" (e.g., "sci,_usi") but omit the "version" key, then instantiate
cloudprofilesync.NewOCI(...) and call oci.GetVersions(ctx) expecting an error
(Expect(err).To(HaveOccurred()) and
Expect(err.Error()).To(ContainSubstring("version annotation not found"))). This
targets the logic in GetVersions/NewOCI that currently assumes a version
annotation and verifies proper error handling when the "version" annotation is
absent.
In `@cloudprofilesync/source.go`:
- Around line 21-29: validFeatureValues is a package-level mutable map; make it
effectively immutable and document the keys: replace the open var usage by
keeping the literal data private but return copies via an accessor (e.g.,
implement a ValidFeatureValues() or GetValidFeatureValues() function that
constructs/returns a fresh map or returns a read-only view) so callers cannot
mutate the package allowlist, and add a short comment above the original
validFeatureValues definition enumerating what each key (e.g., "chost", "_pxe",
"sci", etc.) means in the context of GEP-33; this preserves current behavior
while preventing accidental runtime modification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fb34c5e-0194-4526-b329-99a509a77774
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cloudprofilesync/imageupdater.gocloudprofilesync/imageupdater_test.gocloudprofilesync/provider.gocloudprofilesync/provider_test.gocloudprofilesync/source.gocloudprofilesync/source_test.gocontrollers/managedcloudprofile_controller.gogo.modmain.go
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in feature flag to enable GEP-33 MachineImage capabilities support by parsing additional OCI manifest annotations (feature_set, version) and dual-writing “clean” versions alongside legacy full-tag versions, while also updating dependencies/tooling to newer versions.
Changes:
- Add CLI flag
--enable-machine-image-capabilitiesand plumb it through the controller to the image update pipeline. - Parse OCI manifest annotations into
Capabilities/CleanVersionand (when enabled) dual-write clean versions + providerCapabilityFlavors. - Update Go/tooling/dependencies (
go 1.26.2, gardener/apis + other indirect deps, golangci version regex).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
main.go |
Adds CLI flag and passes it into the controller reconciler. |
controllers/managedcloudprofile_controller.go |
Plumbs the capabilities flag into ImageUpdater and provider implementation. |
cloudprofilesync/source.go |
Parses feature_set/version annotations into Capabilities and CleanVersion. |
cloudprofilesync/source_test.go |
Adds tests for capability annotation parsing and filtering. |
cloudprofilesync/imageupdater.go |
Adds clean-version dual-write behavior gated by the flag and adjusts filtering/sorting. |
cloudprofilesync/imageupdater_test.go |
Adds tests for flag-off vs flag-on behavior, including clean-version dual write. |
cloudprofilesync/provider.go |
Extends Ironcore provider config generation to optionally write CapabilityFlavors. |
cloudprofilesync/provider_test.go |
Adds tests for legacy-only vs dual-write provider config behavior. |
go.mod |
Bumps Go version and updates direct/indirect module versions. |
go.sum |
Updates module checksums per dependency changes. |
.golangci.yaml |
Relaxes Go version regex to accept patch releases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
|
What changed
feature_setandversionare now parsed alongside the existing architecture annotation.feature_setis filtered against an allowlist(chost, _pxe, sci, capi, scibase, _usi, _usidev)- internal-only values are dropped--enable-machine-image-capabilities=false. Default is false - behavior is identical to today when not setCloudProfile: flag OFF (default, no change)
CloudProfile: flag ON (dual-write)
Migration plan
CapabilityFlavorsappear correctly in the CloudProfileNote: disabling the flag after it was enabled does not auto-remove the new-format entries. A manual CloudProfile patch is needed to clean up after a test run
Summary by CodeRabbit
Release Notes
New Features
--enable-machine-image-capabilitiesCLI flag to enable machine image capability flavor support alongside legacy formatImprovements