🔵 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.

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 featureSetCapabilities and imageFilter.requiredFeatureSetValues to the ManagedCloudProfile API/CRD for OCI-sourced machine image updates.
• Updates the OCI source to derive boolean capabilities from feature_set tokens and to expose raw feature_set tokens for downstream filtering.
• Extends the image updater to apply imageFilter based on raw feature_set values, with accompanying tests.

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.

🟡 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.

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

🟢 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.

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

🔵 Needs a closer look

The updated OCI source test file contains invalid Go syntax ([]string value written as {}) which will fail compilation.

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

🟡 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.

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

🟢 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.

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

🟢 Approval recommended

The functional changes are coherent and covered by updated/new tests, with only minor doc/test-description nits noted.

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

🟡 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.

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

🔵 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.

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

🟡 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.

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

🔵 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.

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() (now validateImageVersions()), 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

🟢 Approval recommended

Only minor naming/documentation inconsistencies remain (test descriptions and a stale comment), with core logic and coverage appearing complete.

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