feat: automate Kubernetes version lifecycle management in CloudProfiles - #43
Open
adziauho wants to merge 7 commits into
Open
feat: automate Kubernetes version lifecycle management in CloudProfiles#43adziauho wants to merge 7 commits into
adziauho wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds Kubernetes version synchronization from Landscape, introduces shared OSSync source contracts, updates OCI and Ironcore integrations, adds controller reconciliation and garbage collection flows, and extends the ManagedCloudProfile API and CRD. ChangesCloudProfile synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedCloudProfile
participant CloudProfileReconciler
participant KubernetesImageUpdater
participant LandscapeKubernetesSource
participant CloudProfile
ManagedCloudProfile->>CloudProfileReconciler: Reconcile configuration
CloudProfileReconciler->>KubernetesImageUpdater: Update CloudProfileSpec
KubernetesImageUpdater->>LandscapeKubernetesSource: FetchVersions(ctx)
LandscapeKubernetesSource-->>KubernetesImageUpdater: Return filtered versions
KubernetesImageUpdater->>CloudProfile: Assign Kubernetes versions
CloudProfileReconciler->>CloudProfile: Patch spec and status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 13
🧹 Nitpick comments (9)
cloudprofilesync/ossync/source/oci/os_source_test.go (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
_usitoSupportInPlaceUpdatemapping.This fixture includes
_usi, but it does not assertversions[0].SupportInPlaceUpdate. Add a true assertion here and a valid-feature fixture without_usithat asserts false. This covers the new source-to-updater contract.Proposed test addition
Expect(versions[0].Capabilities).To(Equal(gardencorev1beta1.Capabilities{ "architecture": {"amd64"}, "feature": {"sci", "_usi"}, })) +Expect(versions[0].SupportInPlaceUpdate).To(BeTrue())🤖 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/ossync/source/oci/os_source_test.go` around lines 104 - 108, Extend the OCI fixture tests around NewOCI to assert that a source entry containing _usi maps versions[0].SupportInPlaceUpdate to true. Add a separate valid-feature fixture without _usi and assert the same field is false, covering both sides of the source-to-updater contract.controllers/cloud_profile.go (1)
38-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving the network calls out of the
CreateOrPatchmutate function.
updateMachineImagescontacts an OCI registry.updateKubernetesVersionscontacts an OCI registry and the GitHub API. Both run inside the mutate closure passed tocontrollerutil.CreateOrPatch. Two consequences follow:
- The closure is not guaranteed to run exactly once. Any future conflict-retry wrapper around
CreateOrPatchrepeats every remote call.- A slow registry or a slow GitHub endpoint holds the closure open while the CloudProfile object is staged for patching, which lengthens the window for a conflicting write.
Resolve the source versions before the
CreateOrPatchcall, then apply the resolved values inside the closure. This also makes the closure pure and easier to test.This is a structural change. Defer it if the current behaviour is acceptable.
🤖 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 `@controllers/cloud_profile.go` around lines 38 - 58, The CreateOrPatch mutate closure currently performs remote calls through updateMachineImages and updateKubernetesVersions. Resolve all machine-image and Kubernetes-version updates before invoking controllerutil.CreateOrPatch, then apply those precomputed values inside the closure while preserving error propagation and existing defaults.controllers/garbage_collection.go (3)
192-195: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry the CloudProfile update on conflict.
deleteVersionsperforms aGetat line 116 and anUpdateat line 192.reconcileCloudProfilepatched the same CloudProfile moments earlier in the same reconcile, and the informer cache may still serve the pre-patchresourceVersion. TheUpdatethen returns aConflicterror.Line 104 handles only
apierrors.IsInvalid, so a conflict propagates up, setsFailedReconcileStatus, and returns an error. The ManagedCloudProfile reports a failure for a transient and expected condition.Wrap the read-modify-write in
retry.RetryOnConflict.♻️ Proposed refactor
+import "k8s.io/client-go/util/retry"- if err := r.Update(ctx, &cp); err != nil { - return err - } - return nil + return r.Update(ctx, &cp)Then wrap the whole
Get-mutate-Updatebody ofdeleteVersionsinretry.RetryOnConflict(retry.DefaultRetry, func() error { ... })so that the retry re-reads the CloudProfile.🤖 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 `@controllers/garbage_collection.go` around lines 192 - 195, Wrap the entire Get-mutate-Update flow in deleteVersions with retry.RetryOnConflict using retry.DefaultRetry, re-reading the CloudProfile on each attempt before applying mutations and calling Update. Return the retry result while preserving the existing invalid-error handling and successful nil result.
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit registry type instead of hostname substring matching.
getRegistryProviderselects the Keppel client when the lowercased registry host containskeppel. A Keppel deployment on a host without that substring falls through toerrors.New("no registry provider found for registry"), and garbage collection then fails for a valid configuration.An explicit
registryTypefield on the OCI source in the API removes the guess. This code moved unchanged during the file split, so treat it as a follow-up rather than a blocker for this PR.🤖 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 `@controllers/garbage_collection.go` around lines 33 - 41, Update getRegistryProvider to select the provider from an explicit registryType field on the OCI source rather than matching “keppel” in the hostname. Propagate the registry type through the caller and return KeppelClient when the configured type is Keppel, while preserving validation for empty or unsupported types.
88-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffThe referenced-version snapshot and the CloudProfile update are not atomic.
getReferencedVersionslists Shoots, thendeleteVersionsupdates the CloudProfile. A Shoot created between the two steps can reference a version that this pass removes. The removal does not delete the registry image, so the effect is a Shoot that references a version absent from its CloudProfile. Gardener then fails to reconcile that Shoot.Two mitigations are available:
- Add a grace period so that only versions older than
MaxAgeplus a buffer are eligible, which shrinks the window.- Re-list the Shoots immediately before the
Updateand abort when the referenced set grew.The current 5-minute requeue does not close the gap, because the next pass makes the same decision.
🤖 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 `@controllers/garbage_collection.go` around lines 88 - 101, Make the garbage-collection decision safe against Shoots created after the initial getReferencedVersions snapshot: before updating the CloudProfile, re-list the Shoots and compare the newly referenced set with the original, aborting the deletion/update when it has grown; retain the existing deletion flow only when no new references are detected. Alternatively, enforce a MaxAge grace buffer when selecting versions in the versionsToDelete loop.cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go (2)
145-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the JWT assertions to cover the signature and the claims.
The test only counts three dot-separated segments. It passes even if the signature is invalid,
issis wrong, orexpis missing. GitHub rejects all three cases at runtime, so the test gives little protection for the App authentication path.💚 Proposed fix to verify the signature and the claims
jwt, err := tr.mintJWT() if err != nil { t.Fatalf("unexpected error: %v", err) } - if parts := strings.Split(jwt, "."); len(parts) != 3 { - t.Fatalf("expected 3 JWT parts, got %d", len(parts)) - } + parts := strings.Split(jwt, ".") + if len(parts) != 3 { + t.Fatalf("expected 3 JWT parts, got %d", len(parts)) + } + + sig, err := base64.RawURLEncoding.DecodeString(parts[2]) + if err != nil { + t.Fatalf("decoding signature: %v", err) + } + digest := sha256.Sum256([]byte(parts[0] + "." + parts[1])) + if err := rsa.VerifyPKCS1v15(&key.PublicKey, crypto.SHA256, digest[:], sig); err != nil { + t.Fatalf("signature verification failed: %v", err) + } + + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + t.Fatalf("decoding payload: %v", err) + } + var claims struct { + Iat int64 `json:"iat"` + Exp int64 `json:"exp"` + Iss int64 `json:"iss"` + } + if err := json.Unmarshal(payload, &claims); err != nil { + t.Fatalf("decoding claims: %v", err) + } + if claims.Iss != 42 { + t.Errorf("expected iss 42, got %d", claims.Iss) + } + if claims.Exp <= claims.Iat { + t.Errorf("expected exp %d to be after iat %d", claims.Exp, claims.Iat) + }Add
"crypto","crypto/sha256", and"encoding/base64"to the imports.🤖 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/kubernetessync/source/landscape/landscape_source_test.go` around lines 145 - 156, Strengthen TestGithubAppTransport_MintJWT by parsing the JWT and verifying its signature with the generated key, using SHA-256 and base64url decoding as needed. Assert that the claims include the expected app ID in iss and a valid exp value, while preserving the existing error and three-part checks.
208-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the PKCS8 branches.
parseRSAPrivateKeyhandles aPRIVATE KEYblock and rejects a non-RSA PKCS8 key. Neither branch is tested. Operators commonly convert a GitHub App key to PKCS8, so this path runs in production.💚 Proposed fix to add PKCS8 cases
t.Run("errors on unsupported PEM type", func(t *testing.T) { b := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: []byte("x")}) if _, err := parseRSAPrivateKey(b); err == nil || !strings.Contains(err.Error(), "unsupported") { t.Fatalf("expected unsupported error, got %v", err) } }) + t.Run("parses PKCS8 PEM", func(t *testing.T) { + key, _ := generateTestKey(t) + der, err := x509.MarshalPKCS8PrivateKey(key) + if err != nil { + t.Fatalf("marshalling PKCS8: %v", err) + } + b := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + if _, err := parseRSAPrivateKey(b); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + t.Run("errors on a non-RSA PKCS8 key", func(t *testing.T) { + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generating ed25519 key: %v", err) + } + der, err := x509.MarshalPKCS8PrivateKey(priv) + if err != nil { + t.Fatalf("marshalling PKCS8: %v", err) + } + b := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + if _, err := parseRSAPrivateKey(b); err == nil || !strings.Contains(err.Error(), "not RSA") { + t.Fatalf("expected not-RSA error, got %v", err) + } + })Add
"crypto/ed25519"to the imports.🤖 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/kubernetessync/source/landscape/landscape_source_test.go` around lines 208 - 226, Extend TestParseRSAPrivateKey with PKCS8 coverage: generate a valid RSA private key encoded in a PRIVATE KEY PEM block and assert parseRSAPrivateKey succeeds, then generate an ed25519 key, encode it as PKCS8 in the same PEM type, and assert parsing fails with the expected non-RSA error. Add the crypto/ed25519 import needed for the unsupported-key case.controllers/managedcloudprofile_controller_test.go (1)
163-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a provider-config variant and set
Type.Two small improvements apply to this builder:
- Four fixtures wrap the builder in an immediately invoked function only to attach
ProviderConfig: lines 933-945, 1034-1047, 1137-1149, and 1227-1238. A second helper removes that repetition.- The doc comment states "a minimal valid CloudProfileSpec", but
Typeis never set.CloudProfileSpec.Typecarries no+optionalmarker, so the generated CloudProfile hastype: "". Setting a value makes the fixtures represent a CloudProfile that real Gardener admission accepts.♻️ Proposed refactor
func baseCloudProfileSpec(machineImages ...gardenerv1beta1.MachineImage) v1alpha1.CloudProfileSpec { amd64 := "amd64" usable := true spec := v1alpha1.CloudProfileSpec{ + Type: "ironcore-metal", Regions: []gardenerv1beta1.Region{ { Name: "foo", }, },// baseCloudProfileSpecWithProviderConfig returns baseCloudProfileSpec with the // given raw provider configuration attached. func baseCloudProfileSpecWithProviderConfig(raw []byte, machineImages ...gardenerv1beta1.MachineImage) v1alpha1.CloudProfileSpec { spec := baseCloudProfileSpec(machineImages...) spec.ProviderConfig = &runtime.RawExtension{Raw: raw} return spec }Then each fixture simplifies, for example at lines 933-945:
- CloudProfile: func() v1alpha1.CloudProfileSpec { - cp := baseCloudProfileSpec( - gardenerv1beta1.MachineImage{ - Name: "cap-image", - Versions: []gardenerv1beta1.MachineImageVersion{ - {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: rawTag}, Architectures: []string{"amd64"}}, - {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: cleanVersion}, Architectures: []string{"amd64"}}, - }, - }, - ) - cp.ProviderConfig = &runtime.RawExtension{Raw: raw} - return cp - }(), + CloudProfile: baseCloudProfileSpecWithProviderConfig(raw, + gardenerv1beta1.MachineImage{ + Name: "cap-image", + Versions: []gardenerv1beta1.MachineImageVersion{ + {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: rawTag}, Architectures: []string{"amd64"}}, + {ExpirableVersion: gardenerv1beta1.ExpirableVersion{Version: cleanVersion}, Architectures: []string{"amd64"}}, + }, + }, + ),Setting
Typemay require updating the assertion at line 273, which compares the full spec.🤖 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 `@controllers/managedcloudprofile_controller_test.go` around lines 163 - 188, Update baseCloudProfileSpec to set CloudProfileSpec.Type to a valid provider type so generated fixtures satisfy Gardener admission, and adjust the full-spec assertion near the existing base builder tests. Add a baseCloudProfileSpecWithProviderConfig helper that accepts raw provider configuration, delegates to baseCloudProfileSpec, and attaches it via ProviderConfig; replace the four immediately invoked fixture builders with this helper.cloudprofilesync/kubernetessync/source/landscape/landscape_source.go (1)
401-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a maintained JWT library instead of hand-rolling RS256 signing.
github.com/golang-jwt/jwt/v5supportsSignMethodRS256for GitHub App tokens and avoids manual base64 encoding, SHA-256 signing, andmustJSONfailures unlessjwt.ParseRSAPrivateKeyFromPEMalready rejects the private key. Use the current implementation if keeping dependencies off this path is a hard preference.🤖 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/kubernetessync/source/landscape/landscape_source.go` around lines 401 - 422, Replace the hand-rolled JWT construction in githubAppTransport.mintJWT with github.com/golang-jwt/jwt/v5, using SigningMethodRS256 and claims for iat, exp, and iss. Sign the token with t.key and preserve the existing error-wrapping and token validity timings; remove the manual header, payload, hashing, and base64-signing logic.
🤖 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/kubernetessync/kubernetes_image_updater.go`:
- Around line 40-44: The expiration cutoff in kubernetes_image_updater.go lines
40-44 must use now plus ku.ExpirationThreshold so versions expiring within the
positive threshold are removed; add tests covering that case and negative
thresholds. In api/v1alpha1/managedcloudprofile.go lines 116-120, add
Kubebuilder validation rejecting durations below 0s. Regenerate
crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml lines 608-612 so
its schema enforces duration(self) >= duration('0s').
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go`:
- Around line 230-306: Extract the version-intersection loop from FetchVersions
into a pure intersectVersions helper and have FetchVersions call it; update the
test to cover that helper directly with ExpirableVersion values, removing the
unused githubSrv and abandoned comments. Split the ref query assertion into a
focused TestFetchGithubFileAppendsRef test around fetchClassification,
preserving the expected ref=v1.2.3 behavior.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 299-305: Limit response-body reads to 2048 bytes in both
fetchGithubFile
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:299-305)
and exchangeInstallationToken
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:439-445)
by wrapping each body with io.LimitReader before reading or decoding; preserve
JSON decoding on the limited reader in exchangeInstallationToken and include the
bounded content in errors.
- Around line 363-399: Add a mutex field to githubAppTransport and use it in
installationToken to guard the cached token check, token exchange, and
cached/expiresAt writes as one critical section. Ensure concurrent callers
cannot race or perform duplicate exchanges, while leaving JWT creation and
existing error behavior unchanged.
- Around line 126-131: Set a finite request timeout on the http.Client assigned
to githubClient in the LandscapeKubernetesSource constructor, rather than
leaving Timeout at its zero value. Choose the project’s established timeout
configuration or an appropriate bounded duration so FetchVersions cannot block
reconciliation indefinitely.
- Around line 183-191: Update the tag-selection logic around slices.MaxFunc to
first filter out every tag rejected by semver.ParseTolerant, then compute the
maximum using only parseable semantic versions and their semver comparison.
Remove the mixed string-comparison fallback, while preserving the existing
result handling when no valid tags remain.
- Around line 320-327: Update the expirable-version mapping loop to set
Classification to nil when v.Classification is empty, while retaining a pointer
to the value for non-empty classifications. Use k8s.io/utils/ptr to construct
the conditional pointer and preserve the existing Version and ExpirationDate
mappings.
In `@controllers/cloud_profile.go`:
- Around line 114-122: Validate that the provider configuration in the update
request is present before proceeding; when update.Provider.IroncoreMetal is nil,
return an explicit configuration error instead of leaving provider nil. Update
the provider-selection logic around ossync.Provider and preserve the existing
IroncoreProvider construction for valid configurations.
- Around line 75-86: Update reconcileCloudProfile to call
patchStatusAndCondition unconditionally after successful reconciliation, rather
than only when op != controllerutil.OperationResultNone, so unchanged
CloudProfiles recover stale Failed status. Preserve the existing success status
and CloudProfileApplied condition values, and verify patchStatusAndCondition
remains idempotent without updating LastTransitionTime or writing status when
nothing changed.
In `@controllers/garbage_collection.go`:
- Around line 198-204: Refactor reconcileGarbageCollection and
getReferencedVersions so the reconcile fetches the ShootList and CloudProfile
once before iterating mcp.Spec.MachineImageUpdates, then passes those snapshots
into getReferencedVersions for per-image filtering. Remove the duplicate
List/Get operations from getReferencedVersions, preserve its referenced-version
results, and verify the manager’s cached-client configuration intentionally
supports the all-Shoot snapshot.
- Around line 256-262: Extend RegistryClient.GetTags and its implementations to
accept the OCI connection parameters, then update reconcileGarbageCollection and
fetchKeppelTags to use them instead of hardcoding secure transport. Pass the
same oci.Params as updateMachineImages, including the password resolved by
getCredential, so insecure registries and authenticated Keppel requests work
consistently; update fake RegistryClient implementations and tests to match the
new signature.
- Around line 177-190: Pass the Shoot-referenced versions set from
reconcileGarbageCollection into deleteVersions, then update deleteVersions so
its cascade-delete predicate preserves any version present in that set before
removing clean versions with no remaining flavors. Add coverage for a
Shoot-referenced clean version whose provider config entry has no capability
flavors.
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 33-73: Add a KubernetesVersionSourceFactory field to Reconciler,
mirroring OCISourceFactory, and update updateKubernetesVersions to obtain its
source through that injectable factory instead of calling landscapeSetupSource
directly. Provide fake and configurable test sources, then add coverage for
writing Kubernetes versions, expiration filtering, missing source configuration,
missing GitHub credentials, and PAT/GitHub App credential resolution. Keep the
existing KubernetesVersionUpdateConfig-absent behavior unchanged.
---
Nitpick comments:
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source_test.go`:
- Around line 145-156: Strengthen TestGithubAppTransport_MintJWT by parsing the
JWT and verifying its signature with the generated key, using SHA-256 and
base64url decoding as needed. Assert that the claims include the expected app ID
in iss and a valid exp value, while preserving the existing error and three-part
checks.
- Around line 208-226: Extend TestParseRSAPrivateKey with PKCS8 coverage:
generate a valid RSA private key encoded in a PRIVATE KEY PEM block and assert
parseRSAPrivateKey succeeds, then generate an ed25519 key, encode it as PKCS8 in
the same PEM type, and assert parsing fails with the expected non-RSA error. Add
the crypto/ed25519 import needed for the unsupported-key case.
In `@cloudprofilesync/kubernetessync/source/landscape/landscape_source.go`:
- Around line 401-422: Replace the hand-rolled JWT construction in
githubAppTransport.mintJWT with github.com/golang-jwt/jwt/v5, using
SigningMethodRS256 and claims for iat, exp, and iss. Sign the token with t.key
and preserve the existing error-wrapping and token validity timings; remove the
manual header, payload, hashing, and base64-signing logic.
In `@cloudprofilesync/ossync/source/oci/os_source_test.go`:
- Around line 104-108: Extend the OCI fixture tests around NewOCI to assert that
a source entry containing _usi maps versions[0].SupportInPlaceUpdate to true.
Add a separate valid-feature fixture without _usi and assert the same field is
false, covering both sides of the source-to-updater contract.
In `@controllers/cloud_profile.go`:
- Around line 38-58: The CreateOrPatch mutate closure currently performs remote
calls through updateMachineImages and updateKubernetesVersions. Resolve all
machine-image and Kubernetes-version updates before invoking
controllerutil.CreateOrPatch, then apply those precomputed values inside the
closure while preserving error propagation and existing defaults.
In `@controllers/garbage_collection.go`:
- Around line 192-195: Wrap the entire Get-mutate-Update flow in deleteVersions
with retry.RetryOnConflict using retry.DefaultRetry, re-reading the CloudProfile
on each attempt before applying mutations and calling Update. Return the retry
result while preserving the existing invalid-error handling and successful nil
result.
- Around line 33-41: Update getRegistryProvider to select the provider from an
explicit registryType field on the OCI source rather than matching “keppel” in
the hostname. Propagate the registry type through the caller and return
KeppelClient when the configured type is Keppel, while preserving validation for
empty or unsupported types.
- Around line 88-101: Make the garbage-collection decision safe against Shoots
created after the initial getReferencedVersions snapshot: before updating the
CloudProfile, re-list the Shoots and compare the newly referenced set with the
original, aborting the deletion/update when it has grown; retain the existing
deletion flow only when no new references are detected. Alternatively, enforce a
MaxAge grace buffer when selecting versions in the versionsToDelete loop.
In `@controllers/managedcloudprofile_controller_test.go`:
- Around line 163-188: Update baseCloudProfileSpec to set CloudProfileSpec.Type
to a valid provider type so generated fixtures satisfy Gardener admission, and
adjust the full-spec assertion near the existing base builder tests. Add a
baseCloudProfileSpecWithProviderConfig helper that accepts raw provider
configuration, delegates to baseCloudProfileSpec, and attaches it via
ProviderConfig; replace the four immediately invoked fixture builders with this
helper.
🪄 Autofix
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 Plus
Run ID: 931d66fb-dadb-465f-8997-fedeaae76b1c
📒 Files selected for processing (19)
api/v1alpha1/managedcloudprofile.goapi/v1alpha1/zz_generated.deepcopy.gocloudprofilesync/kubernetessync/kubernetes_image_updater.gocloudprofilesync/kubernetessync/source/landscape/landscape_source.gocloudprofilesync/kubernetessync/source/landscape/landscape_source_test.gocloudprofilesync/ossync/os_image_updater.gocloudprofilesync/ossync/os_image_updater_test.gocloudprofilesync/ossync/provider/ironcore/provider.gocloudprofilesync/ossync/provider/ironcore/provider_test.gocloudprofilesync/ossync/source/oci/os_source.gocloudprofilesync/ossync/source/oci/os_source_test.gocloudprofilesync/ossync/source/oci/suite_test.gocloudprofilesync/ossync/suite_test.gocontrollers/cloud_profile.gocontrollers/garbage_collection.gocontrollers/managedcloudprofile_controller.gocontrollers/managedcloudprofile_controller_test.gocrd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yamlgo.mod
Comment on lines
+40
to
+44
| deleteThreshold := time.Now().Add(-ku.ExpirationThreshold) | ||
| cpVersions := make([]gardenerv1beta1.ExpirableVersion, 0, len(versions)) | ||
| for _, v := range versions { | ||
| if v.ExpirationDate != nil && v.ExpirationDate.Time.Before(deleteThreshold) { //nolint:staticcheck | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the expiration cutoff and reject negative thresholds.
A positive threshold must remove versions that expire before now + threshold. The current now - threshold cutoff only removes versions expired longer than the threshold.
cloudprofilesync/kubernetessync/kubernetes_image_updater.go#L40-L44: usetime.Now().Add(ku.ExpirationThreshold)for the cutoff.api/v1alpha1/managedcloudprofile.go#L116-L120: add a Kubebuilder validation marker that rejects durations below0s.crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml#L608-L612: regenerate the CRD withduration(self) >= duration('0s').
Add tests for a version that expires inside the threshold and for a negative threshold.
📍 Affects 3 files
cloudprofilesync/kubernetessync/kubernetes_image_updater.go#L40-L44(this comment)api/v1alpha1/managedcloudprofile.go#L116-L120crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml#L608-L612
🤖 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/kubernetessync/kubernetes_image_updater.go` around lines 40
- 44, The expiration cutoff in kubernetes_image_updater.go lines 40-44 must use
now plus ku.ExpirationThreshold so versions expiring within the positive
threshold are removed; add tests covering that case and negative thresholds. In
api/v1alpha1/managedcloudprofile.go lines 116-120, add Kubebuilder validation
rejecting durations below 0s. Regenerate
crd/cloudprofilesync.cobaltcore.dev_managedcloudprofiles.yaml lines 608-612 so
its schema enforces duration(self) >= duration('0s').
Comment on lines
+230
to
+306
| // TestFetchVersions_IntersectsAndFilters tests that FetchVersions returns only | ||
| // the classification entries whose versions are present in the OCI descriptor, | ||
| // using a fake HTTP server for the GitHub side and a pre-built source struct. | ||
| func TestFetchVersions_IntersectsAndFilters(t *testing.T) { | ||
| // GitHub server: serves testProvidersYAML (versions 1.31.4, 1.32.1, 1.33.0) | ||
| githubSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if _, err := w.Write([]byte(testProvidersYAML)); err != nil { | ||
| t.Error(err) | ||
| } | ||
| })) | ||
| defer githubSrv.Close() | ||
|
|
||
| // OCI descriptor: only 1.31.4 and 1.32.1 are present (1.33.0 is absent). | ||
| // We inject the supported versions directly via fetchSupportedVersions bypass: | ||
| // build a source whose githubClient points at the fake server, then call | ||
| // fetchClassification + intersection logic manually through FetchVersions by | ||
| // overriding the ociRepo with a pre-baked component descriptor. | ||
| // | ||
| // Because ociRepo requires a real registry, we test the intersection logic | ||
| // indirectly: construct a source with a nil ociRepo but override the | ||
| // fetchSupportedVersions path by testing the public FetchVersions contract | ||
| // through a helper that skips the OCI network call. | ||
| // | ||
| // Instead, test the intersection logic via a thin wrapper that provides a | ||
| // fixed set of supported versions. | ||
| supported := []string{"1.31.4", "1.32.1"} | ||
|
|
||
| classification, err := parseProviderVersions([]byte(testProvidersYAML), "converged-cloud") | ||
| if err != nil { | ||
| t.Fatalf("parse: %v", err) | ||
| } | ||
|
|
||
| supportedSet := make(map[string]bool, len(supported)) | ||
| for _, v := range supported { | ||
| supportedSet[v] = true | ||
| } | ||
|
|
||
| var result []string | ||
| for _, v := range classification { | ||
| if supportedSet[v.Version] { | ||
| result = append(result, v.Version) | ||
| } | ||
| } | ||
|
|
||
| if len(result) != 2 { | ||
| t.Fatalf("expected 2 intersected versions, got %d: %v", len(result), result) | ||
| } | ||
| want := map[string]bool{"1.31.4": true, "1.32.1": true} | ||
| for _, v := range result { | ||
| if !want[v] { | ||
| t.Errorf("unexpected version %q in result", v) | ||
| } | ||
| } | ||
|
|
||
| // Also verify that fetchGithubFile appends ?ref= correctly. | ||
| var gotQuery string | ||
| refSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| gotQuery = r.URL.RawQuery | ||
| if _, err := w.Write([]byte(testProvidersYAML)); err != nil { | ||
| t.Error(err) | ||
| } | ||
| })) | ||
| defer refSrv.Close() | ||
|
|
||
| src := &LandscapeKubernetesSource{ | ||
| githubClient: &http.Client{Transport: &patTransport{token: "tok", base: http.DefaultTransport}}, | ||
| fileURL: refSrv.URL, | ||
| provider: "converged-cloud", | ||
| } | ||
| _, err = src.fetchClassification(context.Background(), "v1.2.3") | ||
| if err != nil { | ||
| t.Fatalf("fetchClassification: %v", err) | ||
| } | ||
| if gotQuery != "ref=v1.2.3" { | ||
| t.Errorf("expected ref=v1.2.3 query param, got %q", gotQuery) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test does not exercise FetchVersions, and it carries dead fixtures.
Three problems exist in this test:
- The name and the doc comment state that it tests
FetchVersions. The body never callsFetchVersions. Lines 262-282 reimplement the intersection loop fromFetchVersionslines 153-164 inside the test, then assert on that copy. The assertion always passes regardless of the production behaviour, soFetchVersionshas no regression coverage. githubSrvat lines 235-240 is created and closed but never used.- Lines 242-254 describe abandoned implementation approaches and contradict each other. They are debug artifacts.
Extract the intersection into a pure function and test that function directly. Then keep the ?ref= check as its own test.
♻️ Proposed refactor
In landscape_source.go, extract the intersection:
// intersectVersions returns the classification entries whose version appears in
// supportedVersions.
func intersectVersions(classification []gardenerv1beta1.ExpirableVersion, supportedVersions []string) []gardenerv1beta1.ExpirableVersion {
supported := make(map[string]bool, len(supportedVersions))
for _, v := range supportedVersions {
supported[v] = true
}
result := make([]gardenerv1beta1.ExpirableVersion, 0, len(classification))
for _, v := range classification {
if supported[v.Version] {
result = append(result, v)
}
}
return result
}Then call it from FetchVersions:
- supported := make(map[string]bool, len(supportedVersions))
- for _, v := range supportedVersions {
- supported[v] = true
- }
-
- result := make([]gardenerv1beta1.ExpirableVersion, 0, len(classification))
- for _, v := range classification {
- if !supported[v.Version] {
- continue
- }
- result = append(result, v)
- }
- return result, nil
+ return intersectVersions(classification, supportedVersions), nilReplace this test with two focused tests:
func TestIntersectVersions(t *testing.T) {
classification, err := parseProviderVersions([]byte(testProvidersYAML), "converged-cloud")
if err != nil {
t.Fatalf("parse: %v", err)
}
// The OCI descriptor supports 1.31.4 and 1.32.1; 1.33.0 is absent.
got := intersectVersions(classification, []string{"1.31.4", "1.32.1"})
if len(got) != 2 {
t.Fatalf("expected 2 intersected versions, got %d", len(got))
}
want := map[string]bool{"1.31.4": true, "1.32.1": true}
for _, v := range got {
if !want[v.Version] {
t.Errorf("unexpected version %q in result", v.Version)
}
}
}
func TestFetchGithubFileAppendsRef(t *testing.T) {
var gotQuery string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotQuery = r.URL.RawQuery
if _, err := w.Write([]byte(testProvidersYAML)); err != nil {
t.Error(err)
}
}))
defer srv.Close()
src := &LandscapeKubernetesSource{
githubClient: &http.Client{Transport: &patTransport{token: "tok", base: http.DefaultTransport}},
fileURL: srv.URL,
provider: "converged-cloud",
}
if _, err := src.fetchClassification(context.Background(), "v1.2.3"); err != nil {
t.Fatalf("fetchClassification: %v", err)
}
if gotQuery != "ref=v1.2.3" {
t.Errorf("expected ref=v1.2.3 query param, got %q", gotQuery)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestFetchVersions_IntersectsAndFilters tests that FetchVersions returns only | |
| // the classification entries whose versions are present in the OCI descriptor, | |
| // using a fake HTTP server for the GitHub side and a pre-built source struct. | |
| func TestFetchVersions_IntersectsAndFilters(t *testing.T) { | |
| // GitHub server: serves testProvidersYAML (versions 1.31.4, 1.32.1, 1.33.0) | |
| githubSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if _, err := w.Write([]byte(testProvidersYAML)); err != nil { | |
| t.Error(err) | |
| } | |
| })) | |
| defer githubSrv.Close() | |
| // OCI descriptor: only 1.31.4 and 1.32.1 are present (1.33.0 is absent). | |
| // We inject the supported versions directly via fetchSupportedVersions bypass: | |
| // build a source whose githubClient points at the fake server, then call | |
| // fetchClassification + intersection logic manually through FetchVersions by | |
| // overriding the ociRepo with a pre-baked component descriptor. | |
| // | |
| // Because ociRepo requires a real registry, we test the intersection logic | |
| // indirectly: construct a source with a nil ociRepo but override the | |
| // fetchSupportedVersions path by testing the public FetchVersions contract | |
| // through a helper that skips the OCI network call. | |
| // | |
| // Instead, test the intersection logic via a thin wrapper that provides a | |
| // fixed set of supported versions. | |
| supported := []string{"1.31.4", "1.32.1"} | |
| classification, err := parseProviderVersions([]byte(testProvidersYAML), "converged-cloud") | |
| if err != nil { | |
| t.Fatalf("parse: %v", err) | |
| } | |
| supportedSet := make(map[string]bool, len(supported)) | |
| for _, v := range supported { | |
| supportedSet[v] = true | |
| } | |
| var result []string | |
| for _, v := range classification { | |
| if supportedSet[v.Version] { | |
| result = append(result, v.Version) | |
| } | |
| } | |
| if len(result) != 2 { | |
| t.Fatalf("expected 2 intersected versions, got %d: %v", len(result), result) | |
| } | |
| want := map[string]bool{"1.31.4": true, "1.32.1": true} | |
| for _, v := range result { | |
| if !want[v] { | |
| t.Errorf("unexpected version %q in result", v) | |
| } | |
| } | |
| // Also verify that fetchGithubFile appends ?ref= correctly. | |
| var gotQuery string | |
| refSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| gotQuery = r.URL.RawQuery | |
| if _, err := w.Write([]byte(testProvidersYAML)); err != nil { | |
| t.Error(err) | |
| } | |
| })) | |
| defer refSrv.Close() | |
| src := &LandscapeKubernetesSource{ | |
| githubClient: &http.Client{Transport: &patTransport{token: "tok", base: http.DefaultTransport}}, | |
| fileURL: refSrv.URL, | |
| provider: "converged-cloud", | |
| } | |
| _, err = src.fetchClassification(context.Background(), "v1.2.3") | |
| if err != nil { | |
| t.Fatalf("fetchClassification: %v", err) | |
| } | |
| if gotQuery != "ref=v1.2.3" { | |
| t.Errorf("expected ref=v1.2.3 query param, got %q", gotQuery) | |
| } | |
| } | |
| func TestIntersectVersions(t *testing.T) { | |
| classification, err := parseProviderVersions([]byte(testProvidersYAML), "converged-cloud") | |
| if err != nil { | |
| t.Fatalf("parse: %v", err) | |
| } | |
| got := intersectVersions(classification, []string{"1.31.4", "1.32.1"}) | |
| if len(got) != 2 { | |
| t.Fatalf("expected 2 intersected versions, got %d", len(got)) | |
| } | |
| want := map[string]bool{"1.31.4": true, "1.32.1": true} | |
| for _, v := range got { | |
| if !want[v.Version] { | |
| t.Errorf("unexpected version %q in result", v.Version) | |
| } | |
| } | |
| } | |
| func TestFetchGithubFileAppendsRef(t *testing.T) { | |
| var gotQuery string | |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| gotQuery = r.URL.RawQuery | |
| if _, err := w.Write([]byte(testProvidersYAML)); err != nil { | |
| t.Error(err) | |
| } | |
| })) | |
| defer srv.Close() | |
| src := &LandscapeKubernetesSource{ | |
| githubClient: &http.Client{Transport: &patTransport{token: "tok", base: http.DefaultTransport}}, | |
| fileURL: srv.URL, | |
| provider: "converged-cloud", | |
| } | |
| if _, err := src.fetchClassification(context.Background(), "v1.2.3"); err != nil { | |
| t.Fatalf("fetchClassification: %v", err) | |
| } | |
| if gotQuery != "ref=v1.2.3" { | |
| t.Errorf("expected ref=v1.2.3 query param, got %q", gotQuery) | |
| } | |
| } |
🤖 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/kubernetessync/source/landscape/landscape_source_test.go`
around lines 230 - 306, Extract the version-intersection loop from FetchVersions
into a pure intersectVersions helper and have FetchVersions call it; update the
test to cover that helper directly with ExpirableVersion values, removing the
unused githubSrv and abandoned comments. Split the ref query assertion into a
focused TestFetchGithubFileAppendsRef test around fetchClassification,
preserving the expected ref=v1.2.3 behavior.
Comment on lines
+126
to
+131
| return &LandscapeKubernetesSource{ | ||
| ociRepo: repo, | ||
| githubClient: &http.Client{Transport: gh.Transport}, | ||
| fileURL: contentsURL(gh.RepositoryApiURL, gh.Repository, gh.FilePath), | ||
| provider: gh.Provider, | ||
| }, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set an explicit timeout on the GitHub HTTP client.
githubClient is created without Timeout. FetchVersions runs inside the controller CreateOrPatch mutate function. The controller-runtime context normally carries no deadline. A GitHub endpoint that accepts the connection but never completes the response will block the reconcile worker for an unbounded time.
🛡️ Proposed fix to bound the GitHub request duration
return &LandscapeKubernetesSource{
ociRepo: repo,
- githubClient: &http.Client{Transport: gh.Transport},
+ githubClient: &http.Client{Transport: gh.Transport, Timeout: 30 * time.Second},
fileURL: contentsURL(gh.RepositoryApiURL, gh.Repository, gh.FilePath),
provider: gh.Provider,
}, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return &LandscapeKubernetesSource{ | |
| ociRepo: repo, | |
| githubClient: &http.Client{Transport: gh.Transport}, | |
| fileURL: contentsURL(gh.RepositoryApiURL, gh.Repository, gh.FilePath), | |
| provider: gh.Provider, | |
| }, nil | |
| return &LandscapeKubernetesSource{ | |
| ociRepo: repo, | |
| githubClient: &http.Client{Transport: gh.Transport, Timeout: 30 * time.Second}, | |
| fileURL: contentsURL(gh.RepositoryApiURL, gh.Repository, gh.FilePath), | |
| provider: gh.Provider, | |
| }, nil |
🤖 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/kubernetessync/source/landscape/landscape_source.go` around
lines 126 - 131, Set a finite request timeout on the http.Client assigned to
githubClient in the LandscapeKubernetesSource constructor, rather than leaving
Timeout at its zero value. Choose the project’s established timeout
configuration or an appropriate bounded duration so FetchVersions cannot block
reconciliation indefinitely.
Comment on lines
+299
to
+305
| if resp.StatusCode != http.StatusOK { | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("can't read body, github API returned %d: %w", resp.StatusCode, err) | ||
| } | ||
| return nil, fmt.Errorf("github API returned %d: %s", resp.StatusCode, body) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded error-body reads in landscape_source.go reach a Kubernetes status condition. Both HTTP error paths call io.ReadAll on the response body with no limit, then embed the full body in the returned error. That error propagates through FetchVersions to reconcileCloudProfile, which writes it into the CloudProfileApplied condition message. Kubernetes limits a condition message to 32768 characters, so a large error page from a proxy makes the status patch fail validation and hides the original failure.
cloudprofilesync/kubernetessync/source/landscape/landscape_source.go#L299-L305: wrap the body inio.LimitReader(resp.Body, 2048)infetchGithubFilebefore building thegithub API returned %d: %serror.cloudprofilesync/kubernetessync/source/landscape/landscape_source.go#L439-L445: wrap the body inio.LimitReader(resp.Body, 2048)inexchangeInstallationToken, and keep the JSON decode working on the limited buffer.
📍 Affects 1 file
cloudprofilesync/kubernetessync/source/landscape/landscape_source.go#L299-L305(this comment)cloudprofilesync/kubernetessync/source/landscape/landscape_source.go#L439-L445
🤖 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/kubernetessync/source/landscape/landscape_source.go` around
lines 299 - 305, Limit response-body reads to 2048 bytes in both fetchGithubFile
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:299-305)
and exchangeInstallationToken
(cloudprofilesync/kubernetessync/source/landscape/landscape_source.go:439-445)
by wrapping each body with io.LimitReader before reading or decoding; preserve
JSON decoding on the limited reader in exchangeInstallationToken and include the
bounded content in errors.
Comment on lines
+114
to
+122
| var provider ossync.Provider | ||
| if update.Provider.IroncoreMetal != nil { | ||
| provider = &ironcore.IroncoreProvider{ | ||
| Registry: update.Provider.IroncoreMetal.Registry, | ||
| Repository: update.Provider.IroncoreMetal.Repository, | ||
| ImageName: update.ImageName, | ||
| EnableCapabilities: r.EnableCapabilities, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
An unrecognized provider is silently ignored, unlike an unrecognized source.
The source switch at lines 92-112 returns errors.New("no machine images source configured") when no source matches. The provider block has no equivalent branch. When update.Provider.IroncoreMetal is nil, provider stays nil and ossync.ImageUpdater.Update skips Provider.Configure.
The result is that spec.machineImages receives the new versions while spec.providerConfig is never populated. Shoots then reference image versions that the provider extension cannot resolve to a registry reference. The garbage collection logic in garbage_collection.go also depends on providerConfig capability flavors, so it cannot protect those tags.
Confirm whether a nil provider is a supported configuration. If it is not, fail explicitly.
🐛 Proposed fix to reject an unconfigured provider
var provider ossync.Provider
- if update.Provider.IroncoreMetal != nil {
+ switch {
+ case update.Provider.IroncoreMetal != nil:
provider = &ironcore.IroncoreProvider{
Registry: update.Provider.IroncoreMetal.Registry,
Repository: update.Provider.IroncoreMetal.Repository,
ImageName: update.ImageName,
EnableCapabilities: r.EnableCapabilities,
}
+ default:
+ return errors.New("no machine images provider configured")
}#!/bin/bash
# Description: Check whether MachineImageUpdateProvider has other variants and whether tests rely on a nil provider.
set -euo pipefail
rg -nP -C 10 'MachineImageUpdateProvider struct' --type=go
# Look for test fixtures that set MachineImageUpdates without a Provider stanza.
rg -nP -C 12 'MachineImageUpdates: \[\]v1alpha1\.MachineImageUpdate' --type=go🤖 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 `@controllers/cloud_profile.go` around lines 114 - 122, Validate that the
provider configuration in the update request is present before proceeding; when
update.Provider.IroncoreMetal is nil, return an explicit configuration error
instead of leaving provider nil. Update the provider-selection logic around
ossync.Provider and preserve the existing IroncoreProvider construction for
valid configurations.
Comment on lines
+177
to
+190
| for i := range cp.Spec.MachineImages { | ||
| if cp.Spec.MachineImages[i].Name != imageName { | ||
| continue | ||
| } | ||
| cp.Spec.MachineImages[i].Versions = slices.DeleteFunc(cp.Spec.MachineImages[i].Versions, func(mv gardenerv1beta1.MachineImageVersion) bool { | ||
| if _, exists := versionsToDelete[mv.Version]; exists { | ||
| return true | ||
| } | ||
| // Cascade-delete clean version entry if all its capability flavors were removed. | ||
| // Only entries tracked as clean versions (present in the map) are eligible. | ||
| hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version] | ||
| return isCleanVersion && !hasRemainingFlavors | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
The cascade delete bypasses Shoot reference protection.
getReferencedVersions returns the versions that Shoots reference, and reconcileGarbageCollection uses that set to build versionsToDelete. deleteVersions never receives the referenced set. Line 182 only consults versionsToDelete, and the cascade at lines 187-188 makes an independent decision.
A clean version reaches the cascade with hasRemainingFlavors == false in this case:
- A Shoot references clean version
2254.0.0. - The provider config entry for
2254.0.0has nocapabilityFlavors. A previous garbage collection pass emptied it, or an operator edited the provider config. - Line 141 records
cleanVersionsWithFlavors["2254.0.0"] = false, and line 143 skips the flavor filter. - Line 167 removes the provider config entry. Line 188 removes
2254.0.0fromspec.machineImages.
The Shoot then references a version that its CloudProfile does not contain, and Gardener fails to reconcile that Shoot. The existing test at lines 1204-1278 of managedcloudprofile_controller_test.go exercises the same path without a Shoot, so it does not catch this.
Pass the referenced set into deleteVersions and exclude a referenced version from the cascade.
🐛 Proposed fix
-func (r *Reconciler) deleteVersions(ctx context.Context, cloudProfileName, imageName string, versionsToDelete map[string]struct{}) error {
+func (r *Reconciler) deleteVersions(ctx context.Context, cloudProfileName, imageName string, versionsToDelete, referencedVersions map[string]struct{}) error { // Remove version entries that have no legacy image ref and no remaining flavors.
cfg.MachineImages[i].Versions = slices.DeleteFunc(cfg.MachineImages[i].Versions, func(mv providercfg.MachineImageVersion) bool {
if mv.Image != "" {
// Legacy flat entry — delete if its tag is in versionsToDelete.
idx := strings.LastIndex(mv.Image, ":")
if idx == -1 {
return false
}
_, exists := versionsToDelete[mv.Image[idx+1:]]
return exists
}
+ // Never cascade-delete a version that a Shoot references.
+ if _, isReferenced := referencedVersions[mv.Version]; isReferenced {
+ return false
+ }
// Clean version entry — delete if all flavors were removed.
return !cleanVersionsWithFlavors[mv.Version]
}) cp.Spec.MachineImages[i].Versions = slices.DeleteFunc(cp.Spec.MachineImages[i].Versions, func(mv gardenerv1beta1.MachineImageVersion) bool {
if _, exists := versionsToDelete[mv.Version]; exists {
return true
}
+ // Never cascade-delete a version that a Shoot references.
+ if _, isReferenced := referencedVersions[mv.Version]; isReferenced {
+ return false
+ }
// Cascade-delete clean version entry if all its capability flavors were removed.
// Only entries tracked as clean versions (present in the map) are eligible.
hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version]
return isCleanVersion && !hasRemainingFlavors
})Update the call site in reconcileGarbageCollection:
- if err := r.deleteVersions(ctx, mcp.Name, updates.ImageName, versionsToDelete); err != nil {
+ if err := r.deleteVersions(ctx, mcp.Name, updates.ImageName, versionsToDelete, referencedVersions); err != nil {Add a test that references a clean version from a Shoot while its provider config entry has no flavors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for i := range cp.Spec.MachineImages { | |
| if cp.Spec.MachineImages[i].Name != imageName { | |
| continue | |
| } | |
| cp.Spec.MachineImages[i].Versions = slices.DeleteFunc(cp.Spec.MachineImages[i].Versions, func(mv gardenerv1beta1.MachineImageVersion) bool { | |
| if _, exists := versionsToDelete[mv.Version]; exists { | |
| return true | |
| } | |
| // Cascade-delete clean version entry if all its capability flavors were removed. | |
| // Only entries tracked as clean versions (present in the map) are eligible. | |
| hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version] | |
| return isCleanVersion && !hasRemainingFlavors | |
| }) | |
| } | |
| for i := range cp.Spec.MachineImages { | |
| if cp.Spec.MachineImages[i].Name != imageName { | |
| continue | |
| } | |
| cp.Spec.MachineImages[i].Versions = slices.DeleteFunc(cp.Spec.MachineImages[i].Versions, func(mv gardenerv1beta1.MachineImageVersion) bool { | |
| if _, exists := versionsToDelete[mv.Version]; exists { | |
| return true | |
| } | |
| // Never cascade-delete a version that a Shoot references. | |
| if _, isReferenced := referencedVersions[mv.Version]; isReferenced { | |
| return false | |
| } | |
| // Cascade-delete clean version entry if all its capability flavors were removed. | |
| // Only entries tracked as clean versions (present in the map) are eligible. | |
| hasRemainingFlavors, isCleanVersion := cleanVersionsWithFlavors[mv.Version] | |
| return isCleanVersion && !hasRemainingFlavors | |
| }) | |
| } |
🤖 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 `@controllers/garbage_collection.go` around lines 177 - 190, Pass the
Shoot-referenced versions set from reconcileGarbageCollection into
deleteVersions, then update deleteVersions so its cascade-delete predicate
preserves any version present in that set before removing clean versions with no
remaining flavors. Add coverage for a Shoot-referenced clean version whose
provider config entry has no capability flavors.
Comment on lines
+198
to
+204
| func (r *Reconciler) getReferencedVersions(ctx context.Context, cloudProfileName, imageName string) (map[string]struct{}, error) { | ||
| referenced := make(map[string]struct{}) | ||
|
|
||
| shootList := &gardenerv1beta1.ShootList{} | ||
| if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err != nil { | ||
| return nil, fmt.Errorf("failed to list Shoots: %w", err) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The full Shoot list runs once per machine image update.
reconcileGarbageCollection calls getReferencedVersions inside the mcp.Spec.MachineImageUpdates loop at line 88. Each call lists every Shoot in every namespace with no field selector and filters client-side. A ManagedCloudProfile with N machine image updates performs N full Shoot listings per reconcile, every 5 minutes.
getReferencedVersions also re-fetches the same CloudProfile that deleteVersions fetches at line 116, so each image costs two additional CloudProfile reads.
List the Shoots once per reconcile, then derive the per-image referenced set from that snapshot. This also removes the inconsistency of taking a different Shoot snapshot for each image.
⚡ Proposed refactor sketch
func (r *Reconciler) reconcileGarbageCollection(ctx context.Context, mcp *v1alpha1.ManagedCloudProfile) error {
if mcp.Spec.GarbageCollection == nil || !mcp.Spec.GarbageCollection.Enabled {
return nil
}
if mcp.Spec.GarbageCollection.MaxAge.Duration < 0 {
return r.failWithStatusUpdate(ctx, mcp, fmt.Errorf("invalid garbage collection maxAge: %s", mcp.Spec.GarbageCollection.MaxAge.String()))
}
cutoff := time.Now().Add(-mcp.Spec.GarbageCollection.MaxAge.Duration)
+
+ shootList := &gardenerv1beta1.ShootList{}
+ if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err != nil {
+ return r.failWithStatusUpdate(ctx, mcp, fmt.Errorf("failed to list Shoots: %w", err))
+ }
+ var cp gardenerv1beta1.CloudProfile
+ if err := r.Get(ctx, types.NamespacedName{Name: mcp.Name}, &cp); err != nil {
+ return r.failWithStatusUpdate(ctx, mcp, fmt.Errorf("failed to get CloudProfile: %w", err))
+ }Change getReferencedVersions to accept the shootList and the fetched cp instead of performing its own reads.
If the manager uses a cached client, also confirm that the Shoot informer cache is acceptable, because it holds every Shoot in memory.
🤖 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 `@controllers/garbage_collection.go` around lines 198 - 204, Refactor
reconcileGarbageCollection and getReferencedVersions so the reconcile fetches
the ShootList and CloudProfile once before iterating
mcp.Spec.MachineImageUpdates, then passes those snapshots into
getReferencedVersions for per-image filtering. Remove the duplicate List/Get
operations from getReferencedVersions, preserve its referenced-version results,
and verify the manager’s cached-client configuration intentionally supports the
all-Shoot snapshot.
Comment on lines
+256
to
+262
| func fetchKeppelTags(ctx context.Context, registry, repository string) (map[string]time.Time, error) { | ||
| baseURL := registryBaseURL(registry, false) | ||
|
|
||
| keppelURL, err := keppelURL(baseURL, repository) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build keppel URL: %w", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
fetchKeppelTags ignores the Insecure setting and sends no credentials.
Two configuration values from updates.Source.OCI never reach this function, because RegistryClient.GetTags accepts only registry and repository:
- Line 257 passes
falseas theinsecureargument, so the scheme is alwayshttps. An operator who setsinsecure: trueon the OCI source, for example for an in-cluster registry without TLS, gets a TLS handshake failure. Garbage collection then fails permanently while machine image synchronization against the same registry succeeds. - No
Authorizationheader is set.UsernameandPasswordfrom the OCI source are not used. A Keppel account that requires authentication returns 401, and garbage collection fails permanently.
The existing tests use a fake RegistryClient, so neither path is covered.
Extend the RegistryClient.GetTags signature to carry the OCI connection parameters.
🐛 Proposed fix sketch
In managedcloudprofile_controller.go:
type RegistryClient interface {
- GetTags(ctx context.Context, registry, repository string) (map[string]time.Time, error)
+ GetTags(ctx context.Context, params oci.Params) (map[string]time.Time, error)
}In garbage_collection.go:
-func (k *KeppelClient) GetTags(ctx context.Context, registry, repository string) (map[string]time.Time, error) {
- return fetchKeppelTags(ctx, registry, repository)
+func (k *KeppelClient) GetTags(ctx context.Context, params oci.Params) (map[string]time.Time, error) {
+ return fetchKeppelTags(ctx, params)
}-func fetchKeppelTags(ctx context.Context, registry, repository string) (map[string]time.Time, error) {
- baseURL := registryBaseURL(registry, false)
+func fetchKeppelTags(ctx context.Context, params oci.Params) (map[string]time.Time, error) {
+ baseURL := registryBaseURL(params.Registry, params.Insecure)
- keppelURL, err := keppelURL(baseURL, repository)
+ keppelURL, err := keppelURL(baseURL, params.Repository) req.Header.Set("Accept", "application/json")
+ if params.Username != "" || params.Password != "" {
+ req.SetBasicAuth(params.Username, params.Password)
+ }Update reconcileGarbageCollection to pass the same oci.Params that updateMachineImages builds, including the resolved password from getCredential. Update the fake clients in managedcloudprofile_controller_test.go to the new signature.
🤖 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 `@controllers/garbage_collection.go` around lines 256 - 262, Extend
RegistryClient.GetTags and its implementations to accept the OCI connection
parameters, then update reconcileGarbageCollection and fetchKeppelTags to use
them instead of hardcoding secure transport. Pass the same oci.Params as
updateMachineImages, including the password resolved by getCredential, so
insecure registries and authenticated Keppel requests work consistently; update
fake RegistryClient implementations and tests to match the new signature.
Comment on lines
33
to
+73
| // fakeSource used to simulate GC list failures in tests | ||
| type fakeSource struct{} | ||
|
|
||
| func (f *fakeSource) GetVersions(ctx context.Context) ([]cloudprofilesync.SourceImage, error) { | ||
| func (f *fakeSource) GetVersions(ctx context.Context) ([]ossync.SourceImage, error) { | ||
| return nil, errors.New("simulated list error") | ||
| } | ||
|
|
||
| // mockOCIFactory implements controllers.OCISourceFactory for testing | ||
| type mockOCIFactory struct { | ||
| createFunc func(params cloudprofilesync.OCIParams, insecure bool) (cloudprofilesync.Source, error) | ||
| createFunc func(params oci.Params, parallel int64) (ossync.Source, error) | ||
| } | ||
|
|
||
| type fakeOCISource struct{} | ||
|
|
||
| func (f *fakeOCISource) GetVersions(ctx context.Context) ([]cloudprofilesync.SourceImage, error) { | ||
| return []cloudprofilesync.SourceImage{ | ||
| func (f *fakeOCISource) GetVersions(ctx context.Context) ([]ossync.SourceImage, error) { | ||
| return []ossync.SourceImage{ | ||
| {Version: "1.0.0", Architectures: []string{"amd64"}}, | ||
| {Version: "1.0.1+abc", Architectures: []string{"amd64"}}, | ||
| }, nil | ||
| } | ||
|
|
||
| type emptyOCISource struct{} | ||
|
|
||
| func (f *emptyOCISource) GetVersions(ctx context.Context) ([]cloudprofilesync.SourceImage, error) { | ||
| func (f *emptyOCISource) GetVersions(ctx context.Context) ([]ossync.SourceImage, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| type fakeFactory struct{} | ||
|
|
||
| func (f *fakeFactory) Create(params cloudprofilesync.OCIParams, insecure bool, _ logr.Logger) (cloudprofilesync.Source, error) { | ||
| func (f *fakeFactory) Create(params oci.Params, _ int64, _ logr.Logger) (ossync.Source, error) { | ||
| return &fakeOCISource{}, nil | ||
| } | ||
|
|
||
| type emptyFactory struct{} | ||
|
|
||
| func (f *emptyFactory) Create(params cloudprofilesync.OCIParams, insecure bool, _ logr.Logger) (cloudprofilesync.Source, error) { | ||
| func (f *emptyFactory) Create(params oci.Params, parallel int64, _ logr.Logger) (ossync.Source, error) { | ||
| return &emptyOCISource{}, nil | ||
| } | ||
|
|
||
| func (m *mockOCIFactory) Create(params cloudprofilesync.OCIParams, insecure bool, _ logr.Logger) (cloudprofilesync.Source, error) { | ||
| return m.createFunc(params, insecure) | ||
| func (m *mockOCIFactory) Create(params oci.Params, parallel int64, _ logr.Logger) (ossync.Source, error) { | ||
| return m.createFunc(params, parallel) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an injectable seam and coverage for the Kubernetes version path.
The fakes cover only the OS image path. OCISourceFactory exists so that updateMachineImages can be tested with fakeFactory, emptyFactory, and mockOCIFactory. The Kubernetes version path has no equivalent. updateKubernetesVersions calls r.landscapeSetupSource directly, which builds a real LandscapeKubernetesSource against a live OCI registry and the GitHub API.
As a result the headline feature of this PR has no positive test. The only new coverage at lines 1280-1301 asserts that nothing happens when KubernetesVersionUpdateConfig is absent. These paths are untested:
- A configured
KubernetesVersionUpdateConfigwrites the source versions intospec.kubernetes.versions. - Version filtering by
ExpirationThreshold. - The
no kubernetes version source configurederror. - The
github source requires personalAccessTokenSecret or githubApperror. - PAT credential resolution and GitHub App credential resolution.
Add a KubernetesVersionSourceFactory field on Reconciler that mirrors OCISourceFactory, then supply a fake source in the tests. The unused KubernetesImageUpdater interface at lines 151-153 of cloud_profile.go suggests this seam was planned.
Do you want me to open an issue to track the missing coverage?
🤖 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 `@controllers/managedcloudprofile_controller_test.go` around lines 33 - 73, Add
a KubernetesVersionSourceFactory field to Reconciler, mirroring
OCISourceFactory, and update updateKubernetesVersions to obtain its source
through that injectable factory instead of calling landscapeSetupSource
directly. Provide fake and configurable test sources, then add coverage for
writing Kubernetes versions, expiration filtering, missing source configuration,
missing GitHub credentials, and PAT/GitHub App credential resolution. Keep the
existing KubernetesVersionUpdateConfig-absent behavior unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements automated Kubernetes version lifecycle management in
cloud-profile-sync, extending the operator to manage Kubernetes versions in CloudProfiles in addition to the existing machine image management.Closes cc/unified-kubernetes#1174 (SAP internal)
Changes
kubernetessyncpackage —KubernetesImageUpdaterwritesspec.kubernetes.versionsto a CloudProfile, filtering out versions whose expiration date has passed the configured thresholdLandscapeKubernetesSource— fetches Kubernetes versions from two sources and merges them:kube-apiserverversionsproviders[].versions[]) for version classifications (supported/deprecated/expired) and expiration dates; supports both PAT and GitHub App (JWT/RSA) authenticationManagedCloudProfileSpecgainskubernetesVersionUpdateConfigwithexpirationThresholdandlandscapeSetup(OCI + GitHub config)managedcloudprofile_controller.gosplit into:cloud_profile.go— CloudProfile reconciliation and machine image / Kubernetes version update logicgarbage_collection.go— GC logic (no functional changes)cloudprofilesync/reorganized intoossync/andkubernetessync/subpackages for cleaner separation of concernsTest plan
KubernetesImageUpdater(expiration threshold filtering)LandscapeKubernetesSource(OCI + GitHub fetch, version merging)KubernetesVersionUpdateConfigSummary by CodeRabbit
New Features
Bug Fixes