kubernetes/controller/internal/configuration/config.go (2)

64-75: Consider using slices.Contains for type checking.

The library's genericv1.Filter function uses slices.Contains for type matching. Using the same pattern here would improve consistency and reduce boilerplate.

♻️ Simplified type check using slices.Contains

+import "slices"
+
 // ...
 
-	for i, entry := range filtered.Configurations {
-		entryType := entry.GetType()
-		isOCMConfig := false
-		for _, t := range ocmConfigTypes {
-			if entryType == t {
-				isOCMConfig = true
-				break
-			}
-		}
-		if !isOCMConfig {
+	for i, entry := range filtered.Configurations {
+		if !slices.Contains(ocmConfigTypes, entry.GetType()) {
 			continue
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config.go` around lines 64 - 75,
Replace the manual loop used to check whether entry.GetType() is one of
ocmConfigTypes with slices.Contains to match the pattern used by
genericv1.Filter: call slices.Contains(ocmConfigTypes, entryType) instead of the
for-loop and isOCMConfig flag, and add the "slices" import if not already
present; update the block that iterates over filtered.Configurations and
references entry.GetType() accordingly.

---

48-52: Consider adding nil-check for defensive safety.

If genericv1.FlatMap returns nil (which can happen in certain edge cases based on the library implementation), genericv1.Filter will panic on config.Type access.

🛡️ Proposed fix to add nil guard

 func filterAllowedConfigTypes(cfg *genericv1.Config) (*genericv1.Config, error) {
+	if cfg == nil {
+		return nil, nil
+	}
 	filtered, err := genericv1.Filter(cfg, &genericv1.FilterOptions{ConfigTypes: allowedConfigTypes})
 	if err != nil {
 		return nil, fmt.Errorf("failed to filter config types: %w", err)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config.go` around lines 48 - 52,
filterAllowedConfigTypes can panic if genericv1.FlatMap returns nil because
genericv1.Filter will access config.Type; before calling genericv1.Filter, call
genericv1.FlatMap(cfg) (or inspect cfg.FlatMap result if available), check that
the returned map/slice is not nil and that entries are non-nil (and have a valid
Type), and if it is nil return a clear error (e.g. fmt.Errorf("empty config
map")) or an empty filtered config; update filterAllowedConfigTypes to perform
this nil-guard around the FlatMap result before invoking genericv1.Filter,
referencing filterAllowedConfigTypes, genericv1.FlatMap, genericv1.Filter, and
allowedConfigTypes.

kubernetes/controller/internal/configuration/config_test.go (1)

664-671: Minor: Test name could be more precise.

The test is named "nil config returns nil configurations" but it actually tests an empty Configurations slice, not a nil *genericv1.Config. Consider renaming to "empty config returns empty configurations" for clarity.

📝 Suggested rename

-	t.Run("nil config returns nil configurations", func(t *testing.T) {
+	t.Run("empty config returns empty configurations", func(t *testing.T) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config_test.go` around lines 664
- 671, The test name is misleading: the t.Run case "nil config returns nil
configurations" actually constructs an empty genericv1.Config and verifies an
empty Configurations slice; rename the test case string to "empty config returns
empty configurations" to reflect intent, keeping the body unchanged (the test
uses cfg := &genericv1.Config{...} and calls filterAllowedConfigTypes), so
update only the t.Run description to the suggested name.

kubernetes/controller/internal/configuration/config_test.go (2)

651-789: Add an explicit nil-config test case for filterAllowedConfigTypes.

The suite is comprehensive, but nil-input behavior isn’t asserted yet. Since nil handling was recently touched, codifying it here would lock in expected behavior.

Proposed test addition

 func TestFilterAllowedConfigTypes(t *testing.T) {
+	t.Run("nil config is handled", func(t *testing.T) {
+		result, err := filterAllowedConfigTypes(nil)
+		require.NoError(t, err)
+		assert.Nil(t, result)
+	})
+
 	makeGenericConfig := func(entries ...string) *genericv1.Config {
 		cfg := &genericv1.Config{
 			Type: ocmruntime.Type{Version: genericv1.Version, Name: genericv1.ConfigType},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config_test.go` around lines 651
- 789, Add a new subtest inside TestFilterAllowedConfigTypes that passes a nil
*genericv1.Config to filterAllowedConfigTypes and asserts it returns no error
and an empty (or nil) Configurations slice; call filterAllowedConfigTypes(nil),
require.NoError(t, err) and require.Empty(t, result.Configurations) (or allow
nil), referencing the existing test harness in TestFilterAllowedConfigTypes and
the function filterAllowedConfigTypes to locate where to add this case.

---

439-444: Consider extracting a fixture helper for repeated credential JSON.

These blocks are repeated with minor value changes. A small helper (e.g., host order input → JSON output) would reduce noise and future update risk.

Also applies to: 459-464, 517-522, 537-542, 595-600, 616-621

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config_test.go` around lines 439
- 444, The test file contains repeated credential JSON blocks (e.g., the inline
objects with "type": "credentials.config.ocm.software/v1" and "consumers":
[{"identities": [{"hostname": "registry-a.io"}], "credentials": []}]) across
multiple cases; extract a small fixture helper (e.g., buildCredentialsJSON or
makeCredentialsFixture that accepts host(s) or host-order and returns the
corresponding JSON string/object) in config_test.go and replace the duplicated
literal blocks with calls to that helper; update tests that currently embed the
repeated snippets (the blocks shown in the diff and the other repeated
occurrences) to use the helper so future edits only change the generator, not
every test occurrence.

kubernetes/controller/internal/configuration/config.go (1)

59-62: Consider moving ocmConfigTypes to package level.

This slice is recreated on every call to filterAllowedConfigTypes. Since it's a constant set of values, it could be defined alongside allowedConfigTypes for a minor efficiency gain.

♻️ Suggested refactor

 var allowedConfigTypes = []runtime.Type{
 	// credentials
 	runtime.NewVersionedType(credentialsv1spec.ConfigType, credentialsv1spec.Version),
 	runtime.NewUnversionedType(credentialsv1spec.ConfigType),
 	// legacy fallback resolvers
 	runtime.NewVersionedType(ocmconfigv1spec.ConfigType, ocmconfigv1spec.Version),
 	runtime.NewUnversionedType(ocmconfigv1spec.ConfigType),
 	// path-matcher resolvers (v1alpha1)
 	runtime.NewVersionedType(resolversv1alpha1spec.ConfigType, resolversv1alpha1spec.Version),
 	runtime.NewUnversionedType(resolversv1alpha1spec.ConfigType),
 }

+// ocmConfigTypes are the types that may contain the deprecated Aliases field.
+var ocmConfigTypes = []runtime.Type{
+	runtime.NewVersionedType(ocmconfigv1spec.ConfigType, ocmconfigv1spec.Version),
+	runtime.NewUnversionedType(ocmconfigv1spec.ConfigType),
+}

Then in filterAllowedConfigTypes, remove lines 59-62 and use the package-level variable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config.go` around lines 59 - 62,
Move the locally-constructed slice ocmConfigTypes out of
filterAllowedConfigTypes and declare it at package level alongside
allowedConfigTypes (e.g., var ocmConfigTypes = []runtime.Type{...} using
runtime.NewVersionedType(...), runtime.NewUnversionedType(...)); then remove the
creation lines from filterAllowedConfigTypes and reference the package-level
ocmConfigTypes there so the constant set of types is reused instead of recreated
on every call.

kubernetes/controller/internal/configuration/config.go (1)

61-74: Consider surfacing how many config entries were dropped by the allowlist.

Silent dropping is good for safety, but a debug/info metric or log of drop count/type would improve incident triage and explain “missing config” behavior during operations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/configuration/config.go` around lines 61 - 74,
The code silently skips configurations not in the ocmConfigTypes allowlist (loop
over filtered.Configurations using slices.Contains(ocmConfigTypes,
entry.GetType())), which makes dropped entries invisible; update the loop to
count and log (or emit a metric) the number of skipped entries by type before
returning so operators can see how many/which config entries were dropped—e.g.,
maintain a map[string]int droppedByType, increment it when an entry is skipped,
and after the loop call the logger/metrics exporter with the aggregate
(reference filtered.Configurations, entry.GetType(), ocmConfigTypes, and
slices.Contains).