Capability-based OCI image filtering - #54
Conversation
|
@coderabbitai review |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CRD/API and reconciliation behavior for machine image selection/filtering, which should be validated by a human reviewer for backward compatibility and operational impact.
Pull request overview
This PR introduces capability-based handling of Gardenlinux feature_set OCI annotations: it can (1) expand selected feature_set tokens into boolean CloudProfile capabilities and (2) filter out source images that don’t satisfy a required set of feature_set values before writing them into the CloudProfile.
Changes:
- Adds
featureSetCapabilitiesandimageFilter.requiredFeatureSetValuesto the ManagedCloudProfile API/CRD for OCI-sourced machine image updates. - Updates the OCI source to derive boolean capabilities from
feature_settokens and to expose raw feature_set tokens for downstream filtering. - Extends the image updater to apply
imageFilterbased on rawfeature_setvalues, with accompanying tests.
File summaries
| File | Description |
|---|---|
| crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml | Extends the CRD schema with featureSetCapabilities and imageFilter.requiredFeatureSetValues. |
| controllers/managedcloudprofile_controller.go | Updates the OCI source factory interface to accept FeatureSetCapability mappings. |
| controllers/managedcloudprofile_controller_test.go | Adapts controller tests to the updated OCI source factory signature. |
| controllers/cloud_profile.go | Wires FeatureSetCapabilities into OCI source creation and passes ImageFilter into the ImageUpdater. |
| cloudprofilesync/ossync/source/oci/os_source.go | Implements boolean capability expansion from raw feature_set tokens and emits RawCapabilities. |
| cloudprofilesync/ossync/source/oci/os_source_test.go | Updates/extends OCI source tests for boolean capabilities and raw token behavior. |
| cloudprofilesync/ossync/os_image_updater.go | Adds ImageFilter handling and uses raw feature_set values to filter images. |
| cloudprofilesync/ossync/os_image_updater_test.go | Adds ImageUpdater integration tests for imageFilter behavior. |
| cloudprofilesync/ossync/image_filter_test.go | Adds focused unit tests for imagePassesFilter. |
| api/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy functions for the new API types/fields. |
| api/v1alpha1/managedcloudprofile.go | Adds API types FeatureSetCapability and ImageFilter and wires them into MachineImageUpdate. |
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
68bb81f to
81b6a8f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The PR introduces a compilation error in os_source_test.go and there is a confirmed goroutine loop-variable capture bug in os_source.go that can cause wrong tag fetches under concurrency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
cloudprofilesync/ossync/source/oci/os_source_test.go:81
- This test case uses an invalid composite literal for an empty []string ("{}").
map[string][]string{"feature_set": {}}does not compile; use[]string{}(or nil) for the slice value.
ossync.SourceImage{
Version: "1.0.1+abc",
Architectures: []string{"amd64"},
RawCapabilities: map[string][]string{"feature_set": {}},
}))
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
81b6a8f to
a938800
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are covered by targeted unit/integration tests and the remaining feedback is limited to minor naming consistency in comments/test descriptions.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
cloudprofilesync/ossync/source/oci/os_source_test.go:318
- Test description mentions "featureSetCapabilities" although the parameter under test is featureToCapabilityMap; aligning the wording avoids confusion.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:356
- Test description mentions "featureSetCapabilities" although the code path is about featureToCapabilityMap being nil/empty; keeping naming consistent improves readability.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The updated OCI source test file contains invalid Go syntax ([]string value written as {}) which will fail compilation.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
cloudprofilesync/ossync/source/oci/os_source.go:31
- This comment still refers to "featureSetCapabilities", but the configuration is now featureToCapabilityMap; the outdated name can mislead readers when tracing behavior.
// annotation directly, independent of the featureSetCapabilities configuration,
cloudprofilesync/ossync/source/oci/os_source_test.go:74
- This map literal uses
{}for an empty []string value, which does not compile in Go. Use an explicit empty slice (and consider using the FeatureSetAnnotation constant for consistency).
Version: "1.0.0",
Architectures: []string{"amd64"},
RawCapabilities: map[string][]string{"feature_set": {}},
}))
cloudprofilesync/ossync/source/oci/os_source_test.go:80
- This map literal uses
{}for an empty []string value, which does not compile in Go. Use an explicit empty slice (and consider using the FeatureSetAnnotation constant for consistency).
ossync.SourceImage{
Version: "1.0.1+abc",
Architectures: []string{"amd64"},
RawCapabilities: map[string][]string{"feature_set": {}},
}))
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
d7e5d9e to
cfa8a91
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The current diff includes a compile-breaking Go test literal ([]string initialized with {}) and a misconfiguration bug where featureToCapabilityMap can overwrite the reserved architecture capability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (5)
cloudprofilesync/ossync/source/oci/os_source_test.go:73
{}is not a valid empty literal for a[]string; this won’t compile. Use[]string{}(or omit the key / use nil) for an empty feature_set slice.
RawCapabilities: map[string][]string{"feature_set": {}},
cloudprofilesync/ossync/source/oci/os_source_test.go:79
{}is not a valid empty literal for a[]string; this won’t compile. Use[]string{}(or omit the key / use nil) for an empty feature_set slice.
RawCapabilities: map[string][]string{"feature_set": {}},
cloudprofilesync/ossync/source/oci/os_source_test.go:83
- The test description mentions "featureSetCapabilities", but the configuration is now called
featureToCapabilityMap; aligning the wording helps readers understand what is being exercised.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:318
- The test description mentions "featureSetCapabilities", but the configuration is now called
featureToCapabilityMap; aligning the wording helps readers understand what is being exercised.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:356
- The test description mentions "featureSetCapabilities", but the configuration is now called
featureToCapabilityMap; aligning the wording helps readers understand what is being exercised.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 10/11 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature-set based filtering/mapping is implemented end-to-end (API → controller wiring → OCI source → updater) with solid test coverage, with only minor wording/spelling nits noted.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
cloudprofilesync/ossync/os_image_updater_test.go:726
- The //nolint comment says "deprecated field", but the suppression here is for an unused parameter; updating the reason will keep future readers from misinterpreting why the lint is disabled.
cloudprofilesync/ossync/os_image_updater.go:22
- The ArchitectureCapability doc comment still references a "capabilityKeys" configuration, but this code path now uses FeatureToCapabilityMap/ImageFilter; the stale wording is confusing for readers.
// ArchitectureCapability is the well-known Gardener capability key for CPU architecture.
// It is read from the "architecture" OCI annotation and excluded from the user-configured
// capabilityKeys since it is always populated automatically by the OCI source.
cloudprofilesync/ossync/os_image_updater.go:242
- Typo in comment: "unnecesscary" -> "unnecessary".
// causing unnecesscary reconciliations.
cloudprofilesync/ossync/source/oci/os_source_test.go:83
- Test description uses the old name "featureSetCapabilities"; the implementation under test is now driven by featureToCapabilityMap.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:318
- Test description uses the old name "featureSetCapabilities"; the behavior now depends on featureToCapabilityMap being nil/empty.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:356
- Test description uses the old name "featureSetCapabilities"; the behavior now depends on featureToCapabilityMap being nil/empty.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
ca87034 to
2d3e434
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are coherent and covered by updated/new tests, with only minor doc/test-description nits noted.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
cloudprofilesync/ossync/os_image_updater.go:21
- The ArchitectureCapability comment still refers to a user-configured "capabilityKeys" setting, but that concept is no longer present (OCI capabilities are now derived via featureToCapabilityMap and/or fixed keys). Updating the comment will avoid confusion when tracing how architecture is populated.
// ArchitectureCapability is the well-known Gardener capability key for CPU architecture.
// It is read from the "architecture" OCI annotation and excluded from the user-configured
// capabilityKeys since it is always populated automatically by the OCI source.
const ArchitectureCapability = "architecture"
cloudprofilesync/ossync/source/oci/os_source_test.go:82
- This test description uses the old term "featureSetCapabilities"; the implementation under test now uses featureToCapabilityMap, so the name is misleading.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:316
- This test description uses the old term "featureSetCapabilities"; the behavior being verified is independent of capabilities configuration (featureToCapabilityMap).
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:354
- This test description uses the old term "featureSetCapabilities"; the test actually checks CleanVersion behavior when featureToCapabilityMap is empty.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
2d3e434 to
bf901b2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed nondeterminism risk in capability generation when multiple feature_set values map to the same capability name (map iteration order), and it should be validated or made deterministic before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
cloudprofilesync/ossync/os_image_updater.go:23
- This comment still refers to a user-configured "capabilityKeys" setting, but this PR switches configuration to FeatureToCapabilityMap; updating the wording will avoid confusion for readers following the new config flow.
// It is read from the "architecture" OCI annotation and excluded from the user-configured
// capabilityKeys since it is always populated automatically by the OCI source.
const ArchitectureCapability = "architecture"
// FeatureSetAnnotation is the gardenlinux OCI annotation key that carries the image's
cloudprofilesync/ossync/source/oci/os_source_test.go:82
- The test name still references the old "featureSetCapabilities" term; the implementation/config is now FeatureToCapabilityMap, so the description should be updated for clarity.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:316
- The test name still references the old "featureSetCapabilities" term; the implementation/config is now FeatureToCapabilityMap, so the description should be updated for clarity.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:354
- The test name still references the old "featureSetCapabilities" term; the implementation/config is now FeatureToCapabilityMap, so the description should be updated for clarity.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Lite
bf901b2 to
2416402
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CRD/API surface area and modifies core machine-image selection behavior (filtering + capability expansion), which warrants final human verification of compatibility and rollout impact.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (5)
cloudprofilesync/ossync/os_image_updater.go:23
- This comment still references "capabilityKeys", but the code path has been updated to use feature_set parsing and featureToCapabilityMap instead. Please update the comment to avoid pointing readers to a removed configuration concept.
// It is read from the "architecture" OCI annotation and excluded from the user-configured
// capabilityKeys since it is always populated automatically by the OCI source.
const ArchitectureCapability = "architecture"
// FeatureSetAnnotation is the gardenlinux OCI annotation key that carries the image's
cloudprofilesync/ossync/source/oci/os_source.go:26
- The comment says this is a "normalized capability value", but the constant is the raw feature_set token ("_usi"). This wording is misleading and contradicts the intention of exact raw feature_set matching used elsewhere in this file.
// usiImageFeature is the normalized capability value for the gardenlinux USI
// (UEFI Secure Image) feature, which indicates support for in-place node updates.
usiImageFeature = "_usi"
cloudprofilesync/ossync/source/oci/os_source_test.go:82
- The test name refers to "featureSetCapabilities", but the implementation/configuration is now featureToCapabilityMap; this makes the test harder to connect to the code being exercised.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:316
- The test name refers to "featureSetCapabilities", but the implementation/configuration is now featureToCapabilityMap; please update to match the current API terminology.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:354
- The test name refers to "featureSetCapabilities", but the implementation/configuration is now featureToCapabilityMap; please update to match the current API terminology.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 8/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
2416402 to
3c1b8a6
Compare
3c1b8a6 to
02e72e3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new featureToCapabilityMap path lacks validation for empty/duplicate capability names (risking invalid/nondeterministic capabilities output), and there is a renamed-function comment mismatch that should be corrected for clarity.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
cloudprofilesync/ossync/source/oci/os_source.go:81
- NewOCI accepts featureToCapabilityMap from the CRD and later iterates it to populate capabilities. As implemented, empty/whitespace capability names will produce a capabilities entry with an empty key, and duplicate capability names can yield nondeterministic results because Go map iteration order is random. Consider validating + normalizing (TrimSpace) the map once in NewOCI and returning an error on empty/duplicate capability names (and empty feature_set keys), storing a sanitized copy on the OCI struct.
func NewOCI(params ocirepo.Params, parallel int64, log logr.Logger, featureToCapabilityMap map[string]string, imageFilter *v1alpha1.ImageFilter) (*OCI, error) {
repo, err := ocirepo.New(params)
if err != nil {
return nil, err
}
cloudprofilesync/ossync/source/oci/os_source.go:26
- The comment for usiImageFeature says it is a "normalized capability value", but the constant now holds the raw feature_set token ("_usi"). This is easy to misread when tracing why supportsInPlaceUpdate requires the underscore form.
// usiImageFeature is the value for the gardenlinux USI (UEFI Secure Image) feature,
// which indicates support for in-place node updates.
usiImageFeature = "_usi"
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The boolean capability expansion can be nondeterministic when multiple feature_set values map to the same capability name due to Go map iteration/overwrite behavior, which should be made deterministic before merge.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
cloudprofilesync/ossync/source/oci/os_source.go:140
- When multiple feature_set values map to the same capabilityName, the current loop can overwrite a previously-set "true" with "false" depending on Go map iteration order (nondeterministic). Also, an empty capabilityName would create a capabilities entry with an empty key.
_, present := rawFeatureSet[featureSetValue]
if present {
caps[capabilityName] = []string{"true"}
} else {
caps[capabilityName] = []string{"false"}
cloudprofilesync/ossync/os_image_updater.go:74
- There is still a comment later in this file that references the old function name
filterImages()(nowvalidateImageVersions()), which can confuse future readers when tracing behavior (see os_image_updater.go:243). Please update that comment to match the new name.
func validateImageVersions(log logr.Logger, versions []SourceImage) []SourceImage {
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
02e72e3 to
fd2f292
Compare
Merging this branch changes the coverage (1 decrease, 2 increase)
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
|
There was a problem hiding this comment.
🟢 Approval recommended
Only minor naming/documentation inconsistencies remain (test descriptions and a stale comment), with core logic and coverage appearing complete.
Review details
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)
cloudprofilesync/ossync/os_image_updater.go:25
- The ArchitectureCapability doc comment still mentions the removed "capabilityKeys" configuration; this is now confusing given the switch to FeatureToCapabilityMap/ImageFilter.
// FeatureSetAnnotation is the gardenlinux OCI annotation key that carries the image's
// feature set as a comma-separated list (e.g. "sci,_usi,vhost").
const FeatureSetAnnotation = "feature_set"
cloudprofilesync/ossync/source/oci/os_source_test.go:82
- The test description still refers to the old "featureSetCapabilities" name; the code uses FeatureToCapabilityMap now, so the test name should be updated to match the current API.
It("expands featureSetCapabilities into boolean capabilities from feature_set annotation", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:316
- This test description still uses the outdated term "featureSetCapabilities"; renaming it to FeatureToCapabilityMap will keep the suite consistent with the new configuration field.
It("detects SupportInPlaceUpdate from feature_set even when featureSetCapabilities is empty", func(ctx SpecContext) {
cloudprofilesync/ossync/source/oci/os_source_test.go:354
- This test description still refers to "featureSetCapabilities" even though the implementation now uses FeatureToCapabilityMap; updating the name will avoid confusion when reading failures.
It("populates CleanVersion from version annotation even when featureSetCapabilities is empty", func(ctx SpecContext) {
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.